Repository navigation
Add file tree sidebar with cmd-click path composition - #728
Closed
AlexBoudreaux wants to merge 2 commits into
Closed
AlexBoudreaux wants to merge 2 commits into
AlexBoudreaux wants to merge 2 commits into
Conversation
Port file tree sidebar from PR manaflow-ai#601 (credit @Shehryar) and add cmd-click to inject file paths into the active terminal pane. - File tree sidebar toggled with Cmd+Shift+E - Toggle or split layout with workspace tabs - Cmd-click any file/folder to insert its shell-escaped path into terminal - Regular click expands/collapses dirs, selects files - Right-click context menu (copy path, reveal in Finder, open, insert path) - Lazy directory loading, hidden files toggle, manual refresh Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@AlexBoudreaux is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a file-tree sidebar feature: five new FileTree Swift modules, a SidebarContentMode state, UI integration in ContentView and App lifecycle, a keyboard shortcut and menu command to toggle between tabs and file-tree modes, and Xcode project updates to include the new files. Changes
Sequence DiagramsequenceDiagram
participant User
participant Menu as Menu/Keyboard
participant App as cmuxApp
participant Delegate as AppDelegate
participant State as SidebarContentModeState
participant View as ContentView
participant Model as FileTreeModel
User->>Menu: Activate "Toggle File Tree" (shortcut/menu)
Menu->>App: invoke command
App->>Delegate: request toggle (via AppDelegate.shared)
Delegate->>State: toggle mode (.tabs ↔ .fileTree)
State->>State: publish new mode
State->>View: EnvironmentObject update
View->>Model: instantiate/load when mode==fileTree
Model->>View: provide rootNodes (async)
View->>User: render FileTreeSidebar or Tabs based on mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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`:
- Line 3320: The app's sidebarContentModeState can become out of sync because
setActiveMainWindow doesn't update it; update sidebarContentModeState inside
setActiveMainWindow to assign the active window's
context.sidebarContentModeState (the same assignment used in the fallback
branches) so the global state always mirrors the newly activated window; ensure
you reference and use the active window's context when setting
sidebarContentModeState so Cmd+Shift+E operates on the correct window mode.
- Around line 5220-5232: The new keyboard shortcut handling for matchShortcut /
KeyboardShortcutSettings.shortcut(for: .toggleFileTree) currently toggles
sidebarContentModeState.mode and sidebarState.toggle() but lacks DEBUG logging;
update the branch inside that if to call dlog(...) in DEBUG builds (use the dlog
free function) to emit a unified debug message indicating the shortcut was
received, the previous and new mode (from sidebarContentModeState.mode) and
whether sidebarState?.isVisible was false (and that you called toggle), so
developers can trace shortcut routing and side-effect (reference matchShortcut,
KeyboardShortcutSettings.shortcut, sidebarContentModeState, and
sidebarState.toggle).
In `@Sources/ContentView.swift`:
- Around line 1865-1889: The split-divider math drifts because totalHeight uses
a hard-coded 30 and the DragGesture adds translation onto tabsHeight (which is
recomputed each event) instead of anchoring to the ratio at drag start; fix by
replacing the magic 30 with a named headerHeight constant (or a measured
headerHeight) when computing totalHeight, and change the gesture to anchor to an
initial drag ratio (e.g., add a `@State` optional dragStartRatio) so on the first
onChanged set dragStartRatio = splitDividerRatio and then compute newRatio =
(dragStartRatio * totalHeight + value.translation.height) / totalHeight, clamped
to 0.15...0.85, and reset dragStartRatio on onEnded; update references to
tabsHeight/totalHeight and the DragGesture handlers (the splitDividerRatio
state, DragGesture .onChanged/.onEnded, and any tabsHeight calculation)
accordingly.
- Around line 1914-1923: The shellEscapePath(_:) function currently
conditionally quotes paths; change it to always wrap the input in single quotes
and escape any inner single quotes by replacing "'" with "'\\''". Locate
shellEscapePath and remove the needsQuoting guard, compute escaped =
path.replacingOccurrences(of: "'", with: "'\\''") and return "'\(escaped)'" for
all inputs so every path is safely single-quoted per the POSIX approach.
In `@Sources/FileTree/FileTreeModel.swift`:
- Around line 130-146: restoreExpandedState currently loads children
asynchronously in restoreExpandedState -> Task but never reapplies the saved
expandedIds to the newly loaded subtree, so nested directories remain collapsed;
fix by, after awaiting self.scanDirectory(path) and assigning children (inside
the Task/@MainActor closure where you call findAndUpdate and set n.children),
call self.restoreExpandedState(in: &n.children, expandedIds: expandedIds) (or an
equivalent helper) so the expansion state is reapplied recursively to the loaded
children; reference restoreExpandedState, scanDirectory, findAndUpdate,
rootNodes and children to locate where to add this recursive reapplication.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/ContentView.swiftSources/FileTree/FileTreeModel.swiftSources/FileTree/FileTreeNode.swiftSources/FileTree/FileTreeRow.swiftSources/FileTree/FileTreeSidebar.swiftSources/FileTree/SidebarContentMode.swiftSources/KeyboardShortcutSettings.swiftSources/cmuxApp.swift
| tabManager = context.tabManager | ||
| sidebarState = context.sidebarState | ||
| sidebarSelectionState = context.sidebarSelectionState | ||
| sidebarContentModeState = context.sidebarContentModeState |
There was a problem hiding this comment.
sidebarContentModeState can drift from the active window context
Line 3320 and Line 3768 only update this state in fallback branches. The primary activation path (setActiveMainWindow) never assigns it, so Cmd+Shift+E can toggle the wrong window mode or no-op after window switches.
🔧 Proposed fix
private func setActiveMainWindow(_ window: NSWindow) {
guard let context = contextForMainTerminalWindow(window) else { return }
`#if` DEBUG
let beforeManagerToken = debugManagerToken(tabManager)
`#endif`
tabManager = context.tabManager
sidebarState = context.sidebarState
sidebarSelectionState = context.sidebarSelectionState
+ sidebarContentModeState = context.sidebarContentModeState
TerminalController.shared.setActiveTabManager(context.tabManager)
`#if` DEBUG
dlog(
"mainWindow.active window={\(debugWindowToken(window))} context={\(debugContextToken(context))} beforeMgr=\(beforeManagerToken) afterMgr=\(debugManagerToken(tabManager)) \(debugShortcutRouteSnapshot())"
)
`#endif`
} if tabManager === removed.tabManager {
// Repoint "active" pointers to any remaining main terminal window.
let nextContext: MainWindowContext? = {
if let keyWindow = NSApp.keyWindow,
let ctx = contextForMainTerminalWindow(keyWindow, reindex: false) {
return ctx
}
return mainWindowContexts.values.first
}()
if let nextContext {
tabManager = nextContext.tabManager
sidebarState = nextContext.sidebarState
sidebarSelectionState = nextContext.sidebarSelectionState
+ sidebarContentModeState = nextContext.sidebarContentModeState
TerminalController.shared.setActiveTabManager(nextContext.tabManager)
} else {
tabManager = nil
sidebarState = nil
sidebarSelectionState = nil
+ sidebarContentModeState = nil
TerminalController.shared.setActiveTabManager(nil)
}
}Also applies to: 3768-3768
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` at line 3320, The app's sidebarContentModeState
can become out of sync because setActiveMainWindow doesn't update it; update
sidebarContentModeState inside setActiveMainWindow to assign the active window's
context.sidebarContentModeState (the same assignment used in the fallback
branches) so the global state always mirrors the newly activated window; ensure
you reference and use the active window's context when setting
sidebarContentModeState so Cmd+Shift+E operates on the correct window mode.
Comment on lines
+5220
to
+5232
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleFileTree)) { | ||
| if let modeState = sidebarContentModeState { | ||
| if modeState.mode == .fileTree { | ||
| modeState.mode = .tabs | ||
| } else { | ||
| modeState.mode = .fileTree | ||
| if sidebarState?.isVisible == false { | ||
| sidebarState?.toggle() | ||
| } | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Add DEBUG dlog() for the new file-tree shortcut path
This adds key-shortcut behavior without unified debug logging, which makes shortcut routing/debugging inconsistent.
📝 Proposed fix
if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleFileTree)) {
+#if DEBUG
+ dlog("shortcut.action name=toggleFileTree \(debugShortcutRouteSnapshot(event: event))")
+#endif
if let modeState = sidebarContentModeState {
if modeState.mode == .fileTree {
modeState.mode = .tabs
} else {
modeState.mode = .fileTree
if sidebarState?.isVisible == false {
sidebarState?.toggle()
}
}
}
return true
}As per coding guidelines, "**/*.swift: All debug events (keys, mouse, focus, splits, tabs) must go to a unified log in DEBUG builds using the dlog() free function".
📝 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
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleFileTree)) { | |
| if let modeState = sidebarContentModeState { | |
| if modeState.mode == .fileTree { | |
| modeState.mode = .tabs | |
| } else { | |
| modeState.mode = .fileTree | |
| if sidebarState?.isVisible == false { | |
| sidebarState?.toggle() | |
| } | |
| } | |
| } | |
| return true | |
| } | |
| if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .toggleFileTree)) { | |
| `#if` DEBUG | |
| dlog("shortcut.action name=toggleFileTree \(debugShortcutRouteSnapshot(event: event))") | |
| `#endif` | |
| if let modeState = sidebarContentModeState { | |
| if modeState.mode == .fileTree { | |
| modeState.mode = .tabs | |
| } else { | |
| modeState.mode = .fileTree | |
| if sidebarState?.isVisible == false { | |
| sidebarState?.toggle() | |
| } | |
| } | |
| } | |
| return true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 5220 - 5232, The new keyboard
shortcut handling for matchShortcut / KeyboardShortcutSettings.shortcut(for:
.toggleFileTree) currently toggles sidebarContentModeState.mode and
sidebarState.toggle() but lacks DEBUG logging; update the branch inside that if
to call dlog(...) in DEBUG builds (use the dlog free function) to emit a unified
debug message indicating the shortcut was received, the previous and new mode
(from sidebarContentModeState.mode) and whether sidebarState?.isVisible was
false (and that you called toggle), so developers can trace shortcut routing and
side-effect (reference matchShortcut, KeyboardShortcutSettings.shortcut,
sidebarContentModeState, and sidebarState.toggle).
Comment on lines
+1865
to
+1889
| let totalHeight = geo.size.height - 30 // account for header | ||
| let tabsHeight = totalHeight * splitDividerRatio | ||
| let fileTreeHeight = totalHeight * (1 - splitDividerRatio) | ||
|
|
||
| VStack(spacing: 0) { | ||
| sidebarModeHeader | ||
|
|
||
| tabsSidebarContent | ||
| .frame(height: tabsHeight) | ||
|
|
||
| // Draggable divider | ||
| Rectangle() | ||
| .fill(Color.primary.opacity(0.1)) | ||
| .frame(height: 1) | ||
| .overlay( | ||
| Rectangle() | ||
| .fill(Color.clear) | ||
| .frame(height: 8) | ||
| .contentShape(Rectangle()) | ||
| .gesture( | ||
| DragGesture() | ||
| .onChanged { value in | ||
| let newRatio = (tabsHeight + value.translation.height) / totalHeight | ||
| splitDividerRatio = min(max(newRatio, 0.15), 0.85) | ||
| } |
There was a problem hiding this comment.
Fix split-divider math to avoid drift and unstable sizing.
Line 1865 uses a hard-coded height offset, and Line 1887 compounds drag translation by adding it to a value already derived from the current ratio. This causes divider drift/jumps during drag and can produce unstable behavior on small heights.
💡 Proposed fix
+ `@State` private var splitDragStartRatio: CGFloat?
+
private var sidebarSplitContent: some View {
- GeometryReader { geo in
- let totalHeight = geo.size.height - 30 // account for header
- let tabsHeight = totalHeight * splitDividerRatio
- let fileTreeHeight = totalHeight * (1 - splitDividerRatio)
-
- VStack(spacing: 0) {
- sidebarModeHeader
+ VStack(spacing: 0) {
+ sidebarModeHeader
+ GeometryReader { geo in
+ let totalHeight = max(geo.size.height, 1)
+ let tabsHeight = totalHeight * splitDividerRatio
+ let fileTreeHeight = totalHeight * (1 - splitDividerRatio)
tabsSidebarContent
.frame(height: tabsHeight)
@@
.gesture(
DragGesture()
.onChanged { value in
- let newRatio = (tabsHeight + value.translation.height) / totalHeight
+ if splitDragStartRatio == nil {
+ splitDragStartRatio = splitDividerRatio
+ }
+ let baseRatio = splitDragStartRatio ?? splitDividerRatio
+ let newRatio = baseRatio + (value.translation.height / totalHeight)
splitDividerRatio = min(max(newRatio, 0.15), 0.85)
}
+ .onEnded { _ in
+ splitDragStartRatio = nil
+ }
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 1865 - 1889, The split-divider math
drifts because totalHeight uses a hard-coded 30 and the DragGesture adds
translation onto tabsHeight (which is recomputed each event) instead of
anchoring to the ratio at drag start; fix by replacing the magic 30 with a named
headerHeight constant (or a measured headerHeight) when computing totalHeight,
and change the gesture to anchor to an initial drag ratio (e.g., add a `@State`
optional dragStartRatio) so on the first onChanged set dragStartRatio =
splitDividerRatio and then compute newRatio = (dragStartRatio * totalHeight +
value.translation.height) / totalHeight, clamped to 0.15...0.85, and reset
dragStartRatio on onEnded; update references to tabsHeight/totalHeight and the
DragGesture handlers (the splitDividerRatio state, DragGesture
.onChanged/.onEnded, and any tabsHeight calculation) accordingly.
Comment on lines
+1914
to
1923
| private func shellEscapePath(_ path: String) -> String { | ||
| let needsQuoting = path.contains(" ") || path.contains("'") || path.contains("\"") | ||
| || path.contains("(") || path.contains(")") || path.contains("&") | ||
| || path.contains("|") || path.contains(";") || path.contains("$") | ||
| || path.contains("`") || path.contains("!") | ||
| guard needsQuoting else { return path } | ||
| // Use single quotes with escaped inner single quotes | ||
| let escaped = path.replacingOccurrences(of: "'", with: "'\\''") | ||
| return "'\(escaped)'" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the file
fd Sources/ContentView.swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Get the line count to understand file size
wc -l Sources/ContentView.swiftRepository: manaflow-ai/cmux
Length of output: 90
🏁 Script executed:
# Read the specific lines mentioned in the review (1914-1923)
sed -n '1914,1923p' Sources/ContentView.swiftRepository: manaflow-ai/cmux
Length of output: 641
🏁 Script executed:
# Search for all usages of shellEscapePath to understand context
rg "shellEscapePath" -B2 -A2Repository: manaflow-ai/cmux
Length of output: 777
🏁 Script executed:
# Look for any other shell escaping utilities or patterns in the codebase
rg "shell.*escape|escape.*shell" -iRepository: manaflow-ai/cmux
Length of output: 2194
🏁 Script executed:
# Examine the escapeDropForShell implementation in GhosttyTerminalView.swift
rg "escapeDropForShell|fileprivate static func escape" -A 10 Sources/GhosttyTerminalView.swiftRepository: manaflow-ai/cmux
Length of output: 1433
🏁 Script executed:
# Check test cases to see what edge cases are covered
cat tests/test_file_drop_paths.pyRepository: manaflow-ai/cmux
Length of output: 2106
🏁 Script executed:
# Search for any newline handling in existing code
rg "newline|\\\\n" Sources/ContentView.swiftRepository: manaflow-ai/cmux
Length of output: 92
🏁 Script executed:
# Find all callers of shellEscapePath to confirm it's used for untrusted input
rg "shellEscapePath" -B5 -A5Repository: manaflow-ai/cmux
Length of output: 1231
Always quote file paths with single quotes to prevent shell metacharacter injection.
The current implementation uses a character allowlist to decide whether quoting is needed, but misses shell-significant characters including glob chars ([, ], *, ?, \), brackets ({}), and others. Unquoted paths containing these characters will be subject to shell expansion or escape sequences.
Adopt the safer POSIX standard approach: always wrap in single quotes and escape inner single quotes with '\''.
Proposed fix
private func shellEscapePath(_ path: String) -> String {
- let needsQuoting = path.contains(" ") || path.contains("'") || path.contains("\"")
- || path.contains("(") || path.contains(")") || path.contains("&")
- || path.contains("|") || path.contains(";") || path.contains("$")
- || path.contains("`") || path.contains("!")
- guard needsQuoting else { return path }
- // Use single quotes with escaped inner single quotes
- let escaped = path.replacingOccurrences(of: "'", with: "'\\''")
- return "'\(escaped)'"
+ // Always single-quote for literal shell argument safety.
+ // Handles whitespace, glob chars, newlines, and metacharacters.
+ let escaped = path.replacingOccurrences(of: "'", with: "'\\''")
+ return "'\(escaped)'"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 1914 - 1923, The shellEscapePath(_:)
function currently conditionally quotes paths; change it to always wrap the
input in single quotes and escape any inner single quotes by replacing "'" with
"'\\''". Locate shellEscapePath and remove the needsQuoting guard, compute
escaped = path.replacingOccurrences(of: "'", with: "'\\''") and return
"'\(escaped)'" for all inputs so every path is safely single-quoted per the
POSIX approach.
Comment on lines
+10
to
+15
| func loadDirectory(_ path: String) { | ||
| rootPath = path | ||
| Task { | ||
| let nodes = await scanDirectory(path) | ||
| self.rootNodes = nodes | ||
| } |
There was a problem hiding this comment.
Prevent stale async scans from overwriting newer tree state.
Concurrent scans can finish out of order (e.g., quick directory switches / refreshes), causing older results to replace newer rootNodes.
🔧 Proposed fix (token-gate async writes)
`@MainActor`
final class FileTreeModel: ObservableObject {
`@Published` var rootPath: String = ""
`@Published` var rootNodes: [FileTreeNode] = []
`@Published` var showHiddenFiles: Bool = true
+ private var scanGeneration: UInt64 = 0
func loadDirectory(_ path: String) {
rootPath = path
+ scanGeneration &+= 1
+ let generation = scanGeneration
Task {
let nodes = await scanDirectory(path)
+ guard generation == self.scanGeneration, path == self.rootPath else { return }
self.rootNodes = nodes
}
}
func refresh() {
guard !rootPath.isEmpty else { return }
+ let path = rootPath
+ scanGeneration &+= 1
+ let generation = scanGeneration
let expandedIds = collectExpandedIds(rootNodes)
Task {
- let nodes = await scanDirectory(rootPath)
+ let nodes = await scanDirectory(path)
var result = nodes
restoreExpandedState(in: &result, expandedIds: expandedIds)
+ guard generation == self.scanGeneration, path == self.rootPath else { return }
self.rootNodes = result
}
}
}Also applies to: 26-34, 40-48, 138-146
Comment on lines
+130
to
+146
| private func restoreExpandedState(in nodes: inout [FileTreeNode], expandedIds: Set<String>) { | ||
| for i in nodes.indices { | ||
| if expandedIds.contains(nodes[i].id) && nodes[i].isDirectory { | ||
| nodes[i].isExpanded = true | ||
| if nodes[i].children == nil { | ||
| let path = nodes[i].path | ||
| let nodeId = nodes[i].id | ||
| nodes[i].children = [] | ||
| Task { @MainActor [weak self] in | ||
| guard let self else { return } | ||
| let children = await self.scanDirectory(path) | ||
| var current = self.rootNodes | ||
| let _ = self.findAndUpdate(in: ¤t, id: nodeId) { n in | ||
| n.children = children | ||
| } | ||
| self.rootNodes = current | ||
| } |
There was a problem hiding this comment.
Nested expanded directories are not restored after refresh.
When child nodes are loaded asynchronously in restoreExpandedState, the saved expandedIds are not reapplied to that newly loaded subtree, so deeper expansions collapse after refresh.
🔧 Proposed fix (reapply expansion state to loaded children)
if nodes[i].children == nil {
let path = nodes[i].path
let nodeId = nodes[i].id
nodes[i].children = []
Task { `@MainActor` [weak self] in
guard let self else { return }
- let children = await self.scanDirectory(path)
+ var children = await self.scanDirectory(path)
+ self.restoreExpandedState(in: &children, expandedIds: expandedIds)
var current = self.rootNodes
let _ = self.findAndUpdate(in: ¤t, id: nodeId) { n in
n.children = children
}
self.rootNodes = current
}
}📝 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
| private func restoreExpandedState(in nodes: inout [FileTreeNode], expandedIds: Set<String>) { | |
| for i in nodes.indices { | |
| if expandedIds.contains(nodes[i].id) && nodes[i].isDirectory { | |
| nodes[i].isExpanded = true | |
| if nodes[i].children == nil { | |
| let path = nodes[i].path | |
| let nodeId = nodes[i].id | |
| nodes[i].children = [] | |
| Task { @MainActor [weak self] in | |
| guard let self else { return } | |
| let children = await self.scanDirectory(path) | |
| var current = self.rootNodes | |
| let _ = self.findAndUpdate(in: ¤t, id: nodeId) { n in | |
| n.children = children | |
| } | |
| self.rootNodes = current | |
| } | |
| private func restoreExpandedState(in nodes: inout [FileTreeNode], expandedIds: Set<String>) { | |
| for i in nodes.indices { | |
| if expandedIds.contains(nodes[i].id) && nodes[i].isDirectory { | |
| nodes[i].isExpanded = true | |
| if nodes[i].children == nil { | |
| let path = nodes[i].path | |
| let nodeId = nodes[i].id | |
| nodes[i].children = [] | |
| Task { `@MainActor` [weak self] in | |
| guard let self else { return } | |
| var children = await self.scanDirectory(path) | |
| self.restoreExpandedState(in: &children, expandedIds: expandedIds) | |
| var current = self.rootNodes | |
| let _ = self.findAndUpdate(in: ¤t, id: nodeId) { n in | |
| n.children = children | |
| } | |
| self.rootNodes = current | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/FileTree/FileTreeModel.swift` around lines 130 - 146,
restoreExpandedState currently loads children asynchronously in
restoreExpandedState -> Task but never reapplies the saved expandedIds to the
newly loaded subtree, so nested directories remain collapsed; fix by, after
awaiting self.scanDirectory(path) and assigning children (inside the
Task/@MainActor closure where you call findAndUpdate and set n.children), call
self.restoreExpandedState(in: &n.children, expandedIds: expandedIds) (or an
equivalent helper) so the expansion state is reapplied recursively to the loaded
children; reference restoreExpandedState, scanDirectory, findAndUpdate,
rootNodes and children to locate where to add this recursive reapplication.
Contributor
Greptile SummaryAdds a file tree sidebar feature that displays the workspace's current directory structure with lazy loading and cmd-click path composition. The implementation includes:
The PR successfully strips the code editor functionality from the original PR #601 and focuses solely on the file tree browser with path composition, keeping the scope manageable. Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User presses Cmd+Shift+E] --> B{SidebarContentModeState}
B -->|Toggle mode| C[.tabs or .fileTree]
C -->|Show sidebar if hidden| D[SidebarState.toggle]
E[ContentView] --> F{fileTreeLayout}
F -->|.toggle| G[Show tabs OR file tree]
F -->|.split| H[Show tabs AND file tree with divider]
I[FileTreeSidebar] -->|Watch| J[workspace.currentDirectory]
J -->|Directory changes| K[FileTreeModel.loadDirectory]
K --> L[Scan directory async]
L --> M[Display file tree nodes]
N[User clicks file/folder] --> O{Cmd key held?}
O -->|Yes| P[sendPathToFocusedTerminal]
O -->|No + Directory| Q[toggleExpand]
O -->|No + File| R[Select file]
P --> S[shellEscapePath]
S --> T[terminalPanel.sendText]
U[User right-clicks] --> V[Context Menu]
V --> W[Copy Path / Reveal in Finder / Open / Insert Path]
Last reviewed commit: f6e4cb5 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+26
to
+34
| Task { @MainActor [weak self] in | ||
| guard let self else { return } | ||
| let children = await self.scanDirectory(path) | ||
| var current = self.rootNodes | ||
| let _ = self.findAndUpdate(in: ¤t, id: node.id) { n in | ||
| n.children = children | ||
| } | ||
| self.rootNodes = current | ||
| } |
Contributor
There was a problem hiding this comment.
Potential race condition: if user rapidly expands multiple directories, concurrent Tasks could overwrite self.rootNodes and lose updates. Between reading self.rootNodes (line 29) and writing it back (line 33), another Task might have modified the state.
Consider using a serial queue or actor-isolated state updates to prevent concurrent modifications.
Comment on lines
+1914
to
+1918
| private func shellEscapePath(_ path: String) -> String { | ||
| let needsQuoting = path.contains(" ") || path.contains("'") || path.contains("\"") | ||
| || path.contains("(") || path.contains(")") || path.contains("&") | ||
| || path.contains("|") || path.contains(";") || path.contains("$") | ||
| || path.contains("`") || path.contains("!") |
Contributor
There was a problem hiding this comment.
Shell escaping doesn't check for all metacharacters. Missing: *, ?, [, ], {, }, ~, #, <, >, newlines, tabs. While rare in macOS paths, these could cause unexpected shell behavior.
Consider always quoting paths or adding more characters to the check:
Suggested change
| private func shellEscapePath(_ path: String) -> String { | |
| let needsQuoting = path.contains(" ") || path.contains("'") || path.contains("\"") | |
| || path.contains("(") || path.contains(")") || path.contains("&") | |
| || path.contains("|") || path.contains(";") || path.contains("$") | |
| || path.contains("`") || path.contains("!") | |
| let needsQuoting = path.contains(" ") || path.contains("'") || path.contains("\"") | |
| || path.contains("(") || path.contains(")") || path.contains("&") | |
| || path.contains("|") || path.contains(";") || path.contains("$") | |
| || path.contains("`") || path.contains("!") || path.contains("*") | |
| || path.contains("?") || path.contains("[") || path.contains("]") | |
| || path.contains("{") || path.contains("}") || path.contains("~") | |
| || path.contains("#") || path.contains("<") || path.contains(">") |
The SwiftUI WindowGroup body in cmuxApp.swift was not injecting the SidebarContentModeState, causing a fatal crash on launch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
Motivation
When working with AI coding agents (Claude Code, etc), you constantly need to reference file paths. Currently you have to type or paste them manually. Cmd-click path composition lets you browse the project tree and inject paths directly into the terminal with a single gesture.
Design decisions
sendTextrather than clipboard paste to avoid bracketed paste mode artifactsCredit
File tree foundation ported from PR #601 by @Shehryar. This PR strips the code editor panel, rebases on clean main, and adds the cmd-click path composition behavior.
Test plan
./scripts/reload.sh🤖 Generated with Claude Code
Summary by CodeRabbit