Repository navigation
Add workspace switching by dragging folder onto sidebar - #1805
wonbywondev wants to merge 4 commits into
Conversation
Dropping a plain directory from Finder onto the cmux sidebar now opens a terminal in that directory: - Existing workspace with the same path → select it and add a horizontal terminal split inside it - No matching workspace → create a new workspace rooted at the folder Files and filesystem packages (.app, .xcworkspace, .playground, etc.) are silently ignored. Drops on terminal or browser panels are unchanged. Implementation: - FileDropOverlayView.performDragOperation: when no terminal is under the cursor, reads URLs from the pasteboard and delegates to onDrop - FileDropOverlayView.updateDragTarget: returns .copy only when the payload contains at least one plain-directory URL (hasDirectoryPath && !isPackage), keeping cursor affordance consistent with actual acceptance - TabManager.handleSidebarFolderDrop: filters to plain directories via isDirectory+isPackage resource keys, normalises paths with FinderServicePathResolver.orderedUniqueDirectories, then selects+splits or creates a workspace Builds on #1571 (dock-icon folder drag support). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@wonbywondev is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis pull request modifies file drop handling in ContentView to route non-terminal area drops to a new sidebar folder drop handler. It implements workspace-aware folder drag-and-drop in TabManager that filters URLs, matches existing workspaces by directory, or creates new ones. It also adjusts terminal split focus timing when initiated from BrowserPanel sources. Changes
Sequence DiagramsequenceDiagram
participant User as User (Drag Drop)
participant View as ContentView
participant Overlay as FileDropOverlay
participant TabMgr as TabManager
participant Workspace as Workspace
participant DB as Workspace Storage
User->>View: Drag folder to non-terminal area
View->>Overlay: performDragOperation triggered
Overlay->>Overlay: Extract URLs from pasteboard
Overlay->>Overlay: Detect plain directories (not packages)
Overlay->>TabMgr: handleSidebarFolderDrop(urls)
TabMgr->>TabMgr: Filter to first eligible directory
TabMgr->>TabMgr: Resolve & deduplicate path
alt Directory matches existing workspace
TabMgr->>DB: Query for workspace by directory
DB-->>TabMgr: Found matching workspace
TabMgr->>TabMgr: Select workspace (set selectedTabId)
TabMgr->>Workspace: Create terminal split from preferred panel
Workspace-->>TabMgr: Split created
else No matching workspace
TabMgr->>DB: Create new workspace at dropped path
DB-->>TabMgr: Workspace created
TabMgr->>TabMgr: Select new workspace
end
TabMgr-->>View: Return true (drop handled)
View-->>User: Folder processed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 662-669: The fallback that treats any non-terminal area as a
sidebar drop should be restricted to actual sidebar drops: instead of
immediately reading URLs and calling onDrop when guard let terminal fails, first
verify the drop location is within the sidebar view (e.g., use
sender.draggingLocation, convert(_:from:), and sidebarView.bounds.contains(...)
or sidebarView.hitTest) and only then perform the folder-on-sidebar fallback
(read URLs and call onDrop). Apply the same restriction to the similar branch
around the 710-724 logic so only drops landing inside the sidebar region trigger
the folder fallback.
In `@Sources/TabManager.swift`:
- Around line 1161-1165: The comparison inside the tabs.first(where: { workspace
in ... }) closure only trims whitespace and trailing slashes on
workspace.currentDirectory, which can miss equivalent paths; update it to
normalize both workspace.currentDirectory and the incoming path using path
normalization (e.g., create URLs via
URL(fileURLWithPath:).standardized.resolvingSymlinksInPath().path or
NSString.standardizingPath()) and then compare those normalized strings after
removing any trailing slash edge-cases so workspaces with equivalent filesystem
paths (including symlinks or redundant components) are detected and reused.
- Around line 1167-1176: The code currently skips opening a terminal when both
existing.focusedPanelId and any TerminalPanel in existing.panels are nil; change
the logic to create a new terminal panel when panelId is nil, then use that new
panel's id to call existing.newTerminalSplit(from:orientation:). Specifically:
if let panelId = existing.focusedPanelId ?? existing.panels.values.compactMap({
$0 as? TerminalPanel }).first?.id { ... } else { create a new terminal panel on
existing (use the existing factory/adder method your codebase provides to
produce a TerminalPanel), obtain its id, and then call
existing.newTerminalSplit(from: newPanel.id, orientation: .horizontal); handle
the case where newPanel or newTerminalSplit returns nil (log or dlog as in the
DEBUG block) so failure is visible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1085e107-212e-43ff-99be-f6878ee0678b
📒 Files selected for processing (2)
Sources/ContentView.swiftSources/TabManager.swift
| guard let terminal else { | ||
| // No terminal under the drop point — delegate to the folder drop handler | ||
| // (e.g. sidebar or non-terminal window area). | ||
| let urls = sender.draggingPasteboard.readObjects( | ||
| forClasses: [NSURL.self], | ||
| options: [.urlReadingFileURLsOnly: true] | ||
| ) as? [URL] ?? [] | ||
| return onDrop?(urls) ?? false |
There was a problem hiding this comment.
Limit the folder-drop fallback to the actual sidebar.
These branches now treat any non-terminal, non-browser area as a sidebar drop target, so a plain folder dropped on the titlebar strip or other non-terminal gaps will still show .copy and create/select a workspace. That broadens the feature beyond the sidebar-only behavior described for this PR.
Also applies to: 710-724
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 662 - 669, The fallback that treats
any non-terminal area as a sidebar drop should be restricted to actual sidebar
drops: instead of immediately reading URLs and calling onDrop when guard let
terminal fails, first verify the drop location is within the sidebar view (e.g.,
use sender.draggingLocation, convert(_:from:), and
sidebarView.bounds.contains(...) or sidebarView.hitTest) and only then perform
the folder-on-sidebar fallback (read URLs and call onDrop). Apply the same
restriction to the similar branch around the 710-724 logic so only drops landing
inside the sidebar region trigger the folder fallback.
There was a problem hiding this comment.
Thanks for the suggestion, but this guard isn't necessary given how the overlay is structured.
FileDropOverlayView is anchored to contentView.topAnchor, which means the title-bar strip above it is physically unreachable by drag-and-drop. Within the overlay's bounds, the only area that is neither a terminal panel nor a browser panel is the sidebar itself — every other pixel is covered by one of those two panel types.
Added a comment in performDragOperation explaining this so the intent is clear to future readers.
There was a problem hiding this comment.
@wonbywondev Got it—thanks for the clarification. I double-checked the overlay install and the new comment in performDragOperation; since the FileDropOverlayView is anchored to contentView (not the native titlebar) and terminal/browser panels occupy the rest of the content area, the fallback effectively maps to “sidebar only.” No further changes needed.
✏️ Learnings added
Learnt from: wonbywondev
Repo: manaflow-ai/cmux PR: 1805
File: Sources/ContentView.swift:662-672
Timestamp: 2026-03-19T10:48:31.943Z
Learning: Repo manaflow-ai/cmux — Sources/ContentView.swift: FileDropOverlayView is anchored to the window’s contentView (theme frame). Within its bounds, the only non-terminal/non-browser area is the Sidebar, so performDragOperation’s folder-drop fallback (calling onDrop when no terminal/web view is under the cursor) is effectively sidebar-only and does not require additional hit-testing for the titlebar or other regions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T08:16:21.950Z
Learning: Applies to **/TerminalWindowPortal.swift : In `WindowTerminalHostView.hitTest()` in `TerminalWindowPortal.swift`: all divider/sidebar/drag routing must be gated to pointer events only. Do not add work outside the `isPointerEvent` guard as this path is called on every event including keyboard input.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T08:16:21.950Z
Learning: Applies to **/GhosttyTerminalView.swift : `SurfaceSearchOverlay` must be mounted from `GhosttySurfaceScrollView` in `Sources/GhosttyTerminalView.swift` (AppKit portal layer), not from SwiftUI panel containers. Portal-hosted terminal views can sit above SwiftUI during split/workspace churn.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-05T03:20:48.079Z
Learning: Applies to **/*TerminalView.swift : Terminal find layering contract: SurfaceSearchOverlay must be mounted from GhosttySurfaceScrollView in Sources/GhosttyTerminalView.swift (AppKit portal layer), not from SwiftUI panel containers
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-05T00:05:51.436Z
Learning: Applies to **/GhosttyTerminalView.swift : `SurfaceSearchOverlay` must be mounted from `GhosttySurfaceScrollView` in `Sources/GhosttyTerminalView.swift` (AppKit portal layer), not from SwiftUI panel containers such as `Sources/Panels/TerminalPanelView.swift`
Learnt from: wonbywondev
Repo: manaflow-ai/cmux PR: 1795
File: Sources/ContentView.swift:1392-1395
Timestamp: 2026-03-19T09:21:22.509Z
Learning: Repo manaflow-ai/cmux — Notifications UI: Production notifications are shown via NSPopover (toggleNotificationsPopover in Sources/Update/UpdateTitlebarAccessory.swift). The SidebarSelectionState.selection == .notifications path in Sources/ContentView.swift is test/scaffolding-only (set in AppDelegate for tests) and isn’t used by user actions; .tabs remains active in normal flows. Therefore, folder-drop handling doesn’t need to flip selection.
Learnt from: SuperManfred
Repo: manaflow-ai/cmux PR: 803
File: Sources/Panels/BrowserPopupWindowController.swift:36-38
Timestamp: 2026-03-04T05:11:56.373Z
Learning: In Sources/Panels/BrowserPopupWindowController.swift and Sources/Panels/BrowserPanel.swift, `webView.isInspectable = true` (guarded by `#available(macOS 13.3, *)`) is intentionally enabled in all builds — not just DEBUG — because cmux is a developer tool and full Web Inspector access is desired in production builds as well. Do not flag this as a security concern.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-16T08:02:06.824Z
Learning: In Swift sources, for any panel_id-only route handling in v2PanelMarkBackground(params:) and v2PanelMarkForeground(params:), first attempt v2ResolveTabManager(params:). Use the manager only if it actually owns the panelId; otherwise fall back to AppDelegate.shared?.locateSurface(surfaceId:) to locate the correct TabManager across windows. Apply this pattern to all panel_id-only routes to avoid active-window bias.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 318
File: Sources/Panels/BrowserPanel.swift:0-0
Timestamp: 2026-03-17T05:34:44.905Z
Learning: In manaflow-ai/cmux (PR `#318`), the browser import wizard in Sources/Panels/BrowserPanel.swift now exposes an Additional data checkbox and passes its state into BrowserImportScope.fromSelection so users can select .everything (including when only additional-data is chosen). Unit and UI tests cover the additional‑data‑only mapping.
Learnt from: SuperManfred
Repo: manaflow-ai/cmux PR: 1150
File: Sources/AppDelegate.swift:7394-7401
Timestamp: 2026-03-10T10:24:14.017Z
Learning: Repo: manaflow-ai/cmux — For Cmd+W behavior with browser popups: BrowserPopupPanel.performKeyEquivalent is the primary handler to close popups. AppDelegate.handleCustomShortcut(_:), in its Cmd+W fallback, must check both NSApp.keyWindow and event.window for identifier "cmux.browser-popup" and close it if found, before routing to workspace/settings close logic.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:3491-3493
Timestamp: 2026-03-04T14:06:16.241Z
Learning: For manaflow-ai/cmux PR `#819` (Japanese i18n), keep scope limited to localization changes; UX enhancements like preferring workspace.customTitle in workspaceDisplayName() or altering move-target labels should be handled in a separate follow-up issue.
Learnt from: homanp
Repo: manaflow-ai/cmux PR: 883
File: CLI/cmux.swift:1569-1586
Timestamp: 2026-03-04T22:05:07.913Z
Learning: In manaflow-ai/cmux CLI (CLI/cmux.swift), for parity with existing browser commands, markdown.open intentionally normalizes --surface before resolving --workspace/--window, relying on server-side resolution to disambiguate final routing. Avoid one-off reordering in markdown; consider any change only as a coordinated, cross-command refactor.
Learnt from: outoftime
Repo: manaflow-ai/cmux PR: 1528
File: Sources/GhosttyTerminalView.swift:3220-3228
Timestamp: 2026-03-17T18:25:33.286Z
Learning: Repo: manaflow-ai/cmux — In Sources/GhosttyTerminalView.swift within TerminalSurface.createSurface(for:), when constructing XDG_DATA_DIRS for Fish vendor_conf.d auto-sourcing, treat empty or whitespace-only values from initialEnvironmentOverrides, env, getenv, and ProcessInfo as unset before prefixing the integrationDir. This avoids producing a trailing colon. Keep XDG_DATA_DIRS in protectedStartupEnvironmentKeys so initialEnvironmentOverrides cannot overwrite the prefixed value.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.
Learnt from: debgotwired
Repo: manaflow-ai/cmux PR: 1149
File: Sources/ContentView.swift:3977-3978
Timestamp: 2026-03-10T09:33:29.814Z
Learning: In manaflow-ai/cmux, for Sources/ContentView.swift, do not modify the sync-seeding behavior of scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:). It is shared infrastructure across all submenus. Treat brief initial flashes as consistent with existing submenus. Any UX improvement (for example making seeding synchronous on a forced corpus refresh) should not be implemented in feature-scoped PRs; instead implement and test such UX changes globally in a dedicated follow-up PR.
Learnt from: zlatkoc
Repo: manaflow-ai/cmux PR: 1368
File: Sources/Panels/BrowserPanel.swift:69-69
Timestamp: 2026-03-13T13:46:01.733Z
Learning: Do not wrap engine/brand name literals (e.g., displayName values such as Google, DuckDuckGo, Bing, Kagi, Startpage) in String(localized: ...). These are brand/product names that are not translatable UI text. Localization should apply to generic UI strings (labels, buttons, error messages, etc.). Apply this guideline across Swift source files under Sources/ (notably in BrowserPanel.swift and similar UI/engine-related strings) and flag only brand-name strings that are part of user-facing UI text appropriately for translation scope.
Learnt from: kjb0787
Repo: manaflow-ai/cmux PR: 1461
File: Sources/GhosttyTerminalView.swift:5904-5905
Timestamp: 2026-03-15T19:22:32.330Z
Learning: In Swift files under the Sources directory that manage terminal/scroll behavior, ensure the following: when preserving scroll across workspace switches, save savedScrollRow only if the scrollbar offset is greater than 0 (indicating the user has scrolled up). On restore, call scroll_to_row only if savedScrollRow is non-nil; if it is nil, rely on synchronizeScrollView() to keep bottom-pinned sessions following new output. This pattern should be applied wherever GhosttyTerminalView-like views implement setVisibleInUI(_:) to maintain consistent user scroll state across workspace switches.
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:1162">
P2: Sidebar folder-drop dedupe compares against mutable `currentDirectory` with ad-hoc normalization, which can miss existing workspaces and create duplicates.</violation>
<violation number="2" location="Sources/TabManager.swift:1169">
P2: Handle the no-terminal case when matching an existing workspace. If no terminal panel ID is available, this path returns success without opening any terminal, so the drop appears to do nothing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…overlay scope Issue 3 fix (Major): When a folder is dropped on the sidebar and the matched workspace has no TerminalPanel (e.g. browser-only), panelId was nil so only the workspace selection fired — no terminal opened. Now falls back to creating a new terminal workspace so the user always gets a shell. Also adds explanatory comments for two intentionally-declined review suggestions: - Title-bar guard (Issue 1): FileDropOverlayView is anchored to contentView.topAnchor, so the title-bar strip above it is physically unreachable; no guard needed. - resolvingSymlinksInPath (Issue 2): orderedUniqueDirectories already uses standardizedFileURL which resolves /tmp->private/tmp; the /tmp symlink test already passes without the extra call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/ContentView.swift (1)
662-672:⚠️ Potential issue | 🟡 MinorRestrict the folder-drop fallback to the actual sidebar.
Because the overlay spans the full
contentView, this path still runs outside the sidebar.terminalContentkeeps a non-terminalcustomTitlebarstrip inside that same area on Lines 2139-2145 and 2188-2240, so dropping a plain folder there — or with the sidebar hidden — will still show.copyand invokehandleSidebarFolderDrop(_:). Please gate both the cursor proposal and theonDropfallback on a real sidebar hit test instead of treating every non-terminal, non-browser location as sidebar.Also applies to: 713-727
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 662 - 672, The folder-drop fallback and cursor proposal must only run when the drop actually landed on the sidebar, not every non-terminal/non-browser area; update the early-exit branch around the `guard let terminal` check to perform a real sidebar hit-test (use the sidebar view used by `terminalContent`/your sidebar component) using the drag location from the `NSDraggingInfo` (`sender`) and only then call `onDrop` or `handleSidebarFolderDrop(_:)` and propose the `.copy` cursor; otherwise return false (or the appropriate non-sidebar result). Ensure both the cursor-proposal code path and the `onDrop` fallback reference `terminalContent`/the same sidebar view so they use the same hit-test logic and avoid treating titlebar/customTitlebar areas as sidebar.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 662-672: The folder-drop fallback and cursor proposal must only
run when the drop actually landed on the sidebar, not every
non-terminal/non-browser area; update the early-exit branch around the `guard
let terminal` check to perform a real sidebar hit-test (use the sidebar view
used by `terminalContent`/your sidebar component) using the drag location from
the `NSDraggingInfo` (`sender`) and only then call `onDrop` or
`handleSidebarFolderDrop(_:)` and propose the `.copy` cursor; otherwise return
false (or the appropriate non-sidebar result). Ensure both the cursor-proposal
code path and the `onDrop` fallback reference `terminalContent`/the same sidebar
view so they use the same hit-test logic and avoid treating
titlebar/customTitlebar areas as sidebar.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9c08d3c-47ce-42f7-aceb-c4595d3d1fee
📒 Files selected for processing (2)
Sources/ContentView.swiftSources/TabManager.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/TabManager.swift
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:1185">
P2: Fallback creation for browser-only matches can repeatedly create duplicate workspaces for the same path and also records an unintended transient tab selection in history.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…plit When a folder is dropped on the sidebar and the matched workspace contains only non-terminal panels (e.g. browser-only), the handler now splits a terminal beside the existing panel using the browser panel as the split source, rather than creating a separate workspace. Also prevents duplicate workspace creation and transient tab-selection history pollution on repeated drops to the same browser-only workspace. Re-establishes focus on the new terminal after the split via DispatchQueue.main.async when the split source is a BrowserPanel, to work around the reparent-focus mechanism not applying to non-terminal source panels. A deeper fix in newTerminalSplit is tracked separately. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Phase 11 fix: first-click swallowed after browser-panel splitRoot cause confirmed: After Fix (commit a7b1428):
Tested:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Sources/Panels/BrowserPanel.swift (1)
4748-4754: Add debug underflow visibility for unbalanced suppress/clear calls.Line 4753 clamps to zero, which is safe, but it can hide lifecycle imbalances. A DEBUG-only dlog for underflow makes regressions easier to diagnose.
🔍 Suggested patch
func clearSuppressReparentFocus() { +#if DEBUG + if suppressReparentFocusDepth == 0 { + dlog("browser.focus.reparentSuppress.clearUnderflow panel=\(id.uuidString.prefix(5))") + } +#endif suppressReparentFocusDepth = max(0, suppressReparentFocusDepth - 1) }As per coding guidelines: "All debug events ... use free function
dlog(...)... wrap all call sites in#if DEBUG/#endif."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 4748 - 4754, The clearSuppressReparentFocus method currently clamps suppressReparentFocusDepth to zero, which hides unbalanced clear calls; modify clearSuppressReparentFocus to detect when suppressReparentFocusDepth is already 0 before decrementing and, under a DEBUG build, call the free function dlog(...) to record an underflow/imbalanced lifecycle event (include context like "clearSuppressReparentFocus called with depth == 0"). Wrap the dlog call in `#if` DEBUG / `#endif` and keep the existing clamp behavior (use max(0, ...)) so production behavior is unchanged; reference the functions suppressReparentFocus and clearSuppressReparentFocus to locate the change.
🤖 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/Panels/BrowserPanel.swift`:
- Around line 1884-1887: The suppressReparentFocusDepth flag is not being
cleared during workspace context resets causing sticky focus suppression; update
resetForWorkspaceContextChange(reason:) to reset suppressReparentFocusDepth (set
to 0) or call clearSuppressReparentFocus() alongside the other suppression
fields so that shouldSuppressWebViewFocus() no longer returns true after a
reset.
In `@Sources/TabManager.swift`:
- Around line 1181-1199: The drop handler currently returns true even when no
terminal was created (when panelId is nil or newTerminalSplit(from:orientation:)
returns nil); update the logic in the folder-drop handling block so you only
return true if a terminal/view was actually created — i.e., check panelId and
the result of existing.newTerminalSplit(from:panelId, orientation:) and only
treat the drop as handled when that result is non-nil; if no split was created,
fall through (or return false) so addWorkspace(workingDirectory:select:) is used
or the drop is reported unhandled as appropriate.
In `@Sources/Workspace.swift`:
- Around line 6623-6642: The browser path calls
browserSourcePanel?.suppressReparentFocus() but the paired
clearSuppressReparentFocus() calls live only inside the DispatchQueue.main.async
closure guarded by `guard let self`, so if `self` is deallocated before the
async block runs the suppression is never cleared; move the cleanup so it always
runs (e.g., call previousHostedView?.clearSuppressReparentFocus() and
browserSourcePanel.clearSuppressReparentFocus() in a defer or immediately before
the async dispatch/guard) while keeping the existing
focusPanel(browserSourcePanel.id, previousHostedView: previousHostedView) and
the async focusPanel(newPanel.id, previousHostedView: nil) behavior intact.
---
Nitpick comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 4748-4754: The clearSuppressReparentFocus method currently clamps
suppressReparentFocusDepth to zero, which hides unbalanced clear calls; modify
clearSuppressReparentFocus to detect when suppressReparentFocusDepth is already
0 before decrementing and, under a DEBUG build, call the free function dlog(...)
to record an underflow/imbalanced lifecycle event (include context like
"clearSuppressReparentFocus called with depth == 0"). Wrap the dlog call in `#if`
DEBUG / `#endif` and keep the existing clamp behavior (use max(0, ...)) so
production behavior is unchanged; reference the functions suppressReparentFocus
and clearSuppressReparentFocus to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41e43d4e-b4ab-4561-a01e-a6df9242a69a
📒 Files selected for processing (4)
Sources/BrowserWindowPortal.swiftSources/Panels/BrowserPanel.swiftSources/TabManager.swiftSources/Workspace.swift
| /// Suppresses web-view focus-policy updates during SwiftUI reparenting after a | ||
| /// programmatic split from this browser panel. Mirrors suppressReparentFocus() on | ||
| /// GhosttySurfaceScrollView. Use suppressReparentFocus() / clearSuppressReparentFocus(). | ||
| private var suppressReparentFocusDepth: Int = 0 |
There was a problem hiding this comment.
Reset suppressReparentFocusDepth in workspace-context resets to avoid sticky webview focus suppression.
This new state feeds shouldSuppressWebViewFocus() (Line 4766), but it is not cleared in resetForWorkspaceContextChange(reason:) where other suppression fields are reset. If a clear path is skipped during reparent churn, focus can stay suppressed longer than intended.
💡 Suggested patch
diff --git a/Sources/Panels/BrowserPanel.swift b/Sources/Panels/BrowserPanel.swift
@@
suppressOmnibarAutofocusUntil = nil
suppressWebViewFocusUntil = nil
+ suppressReparentFocusDepth = 0
endSuppressWebViewFocusForAddressBar()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanel.swift` around lines 1884 - 1887, The
suppressReparentFocusDepth flag is not being cleared during workspace context
resets causing sticky focus suppression; update
resetForWorkspaceContextChange(reason:) to reset suppressReparentFocusDepth (set
to 0) or call clearSuppressReparentFocus() alongside the other suppression
fields so that shouldSuppressWebViewFocus() no longer returns true after a
reset.
| if let panelId { | ||
| let newPanel = existing.newTerminalSplit(from: panelId, orientation: .horizontal) | ||
| #if DEBUG | ||
| if newPanel == nil { | ||
| dlog("sidebar.folderDrop.splitFailed panelId=\(panelId.uuidString.prefix(5))") | ||
| } | ||
| #endif | ||
| } | ||
| #if DEBUG | ||
| dlog("sidebar.folderDrop.existing workspaceId=\(existing.id.uuidString.prefix(5))") | ||
| #endif | ||
| } else { | ||
| addWorkspace(workingDirectory: path, select: true) | ||
| #if DEBUG | ||
| dlog("sidebar.folderDrop.new path=\(path)") | ||
| #endif | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Do not report success when no terminal was created.
At Line 1181 and Line 1198, the method can return true even when panelId is nil or newTerminalSplit fails. That marks the drop as handled without opening a shell.
Suggested hardening
if let existing {
selectedTabId = existing.id
// Prefer a focused or existing TerminalPanel as the split source.
// If the workspace has only non-terminal panels (e.g. browser-only), use any panel
// so a new terminal appears as a split beside it rather than creating a new workspace.
let panelId = existing.focusedPanelId
?? existing.panels.values.compactMap { $0 as? TerminalPanel }.first?.id
?? existing.panels.values.first?.id
- if let panelId {
- let newPanel = existing.newTerminalSplit(from: panelId, orientation: .horizontal)
+ if let panelId,
+ existing.newTerminalSplit(from: panelId, orientation: .horizontal) != nil {
`#if` DEBUG
- if newPanel == nil {
- dlog("sidebar.folderDrop.splitFailed panelId=\(panelId.uuidString.prefix(5))")
- }
+ dlog("sidebar.folderDrop.existing workspaceId=\(existing.id.uuidString.prefix(5))")
`#endif`
+ } else {
+ // Ensure drop still opens a terminal if split source/creation fails.
+ _ = addWorkspace(workingDirectory: path, select: true)
+#if DEBUG
+ dlog("sidebar.folderDrop.fallback.new path=\(path)")
+#endif
}
-#if DEBUG
- dlog("sidebar.folderDrop.existing workspaceId=\(existing.id.uuidString.prefix(5))")
-#endif
} else {
addWorkspace(workingDirectory: path, select: true)
`#if` DEBUG
dlog("sidebar.folderDrop.new path=\(path)")
`#endif`
}
return true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 1181 - 1199, The drop handler
currently returns true even when no terminal was created (when panelId is nil or
newTerminalSplit(from:orientation:) returns nil); update the logic in the
folder-drop handling block so you only return true if a terminal/view was
actually created — i.e., check panelId and the result of
existing.newTerminalSplit(from:panelId, orientation:) and only treat the drop as
handled when that result is non-nil; if no split was created, fall through (or
return false) so addWorkspace(workingDirectory:select:) is used or the drop is
reported unhandled as appropriate.
| browserSourcePanel?.suppressReparentFocus() | ||
|
|
||
| if let browserSourcePanel { | ||
| // When the source is a BrowserPanel, the WKWebView portal window still covers | ||
| // the full workspace rect until SwiftUI layout shrinks it after the split. | ||
| // Directly focusing the new terminal causes the first click to be intercepted | ||
| // by the stale portal before it is repositioned. | ||
| // | ||
| // Workaround: focus the browser panel first (so the portal's | ||
| // allowsFirstResponderAcquisition path settles), then focus the terminal | ||
| // on the next runloop after SwiftUI layout has updated the portal geometry. | ||
| // This mirrors the user manually clicking the browser then the terminal, which | ||
| // was confirmed to work correctly. | ||
| focusPanel(browserSourcePanel.id, previousHostedView: previousHostedView) | ||
| DispatchQueue.main.async { [weak self] in | ||
| guard let self else { return } | ||
| self.focusPanel(newPanel.id, previousHostedView: nil) | ||
| previousHostedView?.clearSuppressReparentFocus() | ||
| browserSourcePanel.clearSuppressReparentFocus() | ||
| } |
There was a problem hiding this comment.
Always balance suppressReparentFocus() on the browser path.
The queued block returns before either clear runs when self is gone, so a teardown between suppressReparentFocus() and the next-runloop callback can leave the source browser panel stuck with a positive suppression depth. Move the cleanup into a defer (or before the guard) so it always executes.
Suggested fix
- DispatchQueue.main.async { [weak self] in
- guard let self else { return }
- self.focusPanel(newPanel.id, previousHostedView: nil)
- previousHostedView?.clearSuppressReparentFocus()
- browserSourcePanel.clearSuppressReparentFocus()
- }
+ let newPanelId = newPanel.id
+ DispatchQueue.main.async { [weak self, weak previousHostedView, weak browserSourcePanel] in
+ defer {
+ previousHostedView?.clearSuppressReparentFocus()
+ browserSourcePanel?.clearSuppressReparentFocus()
+ }
+ guard let self else { return }
+ self.focusPanel(newPanelId, previousHostedView: nil)
+ }📝 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.
| browserSourcePanel?.suppressReparentFocus() | |
| if let browserSourcePanel { | |
| // When the source is a BrowserPanel, the WKWebView portal window still covers | |
| // the full workspace rect until SwiftUI layout shrinks it after the split. | |
| // Directly focusing the new terminal causes the first click to be intercepted | |
| // by the stale portal before it is repositioned. | |
| // | |
| // Workaround: focus the browser panel first (so the portal's | |
| // allowsFirstResponderAcquisition path settles), then focus the terminal | |
| // on the next runloop after SwiftUI layout has updated the portal geometry. | |
| // This mirrors the user manually clicking the browser then the terminal, which | |
| // was confirmed to work correctly. | |
| focusPanel(browserSourcePanel.id, previousHostedView: previousHostedView) | |
| DispatchQueue.main.async { [weak self] in | |
| guard let self else { return } | |
| self.focusPanel(newPanel.id, previousHostedView: nil) | |
| previousHostedView?.clearSuppressReparentFocus() | |
| browserSourcePanel.clearSuppressReparentFocus() | |
| } | |
| browserSourcePanel?.suppressReparentFocus() | |
| if let browserSourcePanel { | |
| // When the source is a BrowserPanel, the WKWebView portal window still covers | |
| // the full workspace rect until SwiftUI layout shrinks it after the split. | |
| // Directly focusing the new terminal causes the first click to be intercepted | |
| // by the stale portal before it is repositioned. | |
| // | |
| // Workaround: focus the browser panel first (so the portal's | |
| // allowsFirstResponderAcquisition path settles), then focus the terminal | |
| // on the next runloop after SwiftUI layout has updated the portal geometry. | |
| // This mirrors the user manually clicking the browser then the terminal, which | |
| // was confirmed to work correctly. | |
| focusPanel(browserSourcePanel.id, previousHostedView: previousHostedView) | |
| let newPanelId = newPanel.id | |
| DispatchQueue.main.async { [weak self, weak previousHostedView, weak browserSourcePanel] in | |
| defer { | |
| previousHostedView?.clearSuppressReparentFocus() | |
| browserSourcePanel?.clearSuppressReparentFocus() | |
| } | |
| guard let self else { return } | |
| self.focusPanel(newPanelId, previousHostedView: nil) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 6623 - 6642, The browser path calls
browserSourcePanel?.suppressReparentFocus() but the paired
clearSuppressReparentFocus() calls live only inside the DispatchQueue.main.async
closure guarded by `guard let self`, so if `self` is deallocated before the
async block runs the suppression is never cleared; move the cleanup so it always
runs (e.g., call previousHostedView?.clearSuppressReparentFocus() and
browserSourcePanel.clearSuppressReparentFocus() in a defer or immediately before
the async dispatch/guard) while keeping the existing
focusPanel(browserSourcePanel.id, previousHostedView: previousHostedView) and
the async focusPanel(newPanel.id, previousHostedView: nil) behavior intact.
Root cause: after newTerminalSplit from a BrowserPanel source, the WKWebView portal window still covers the full workspace rect until SwiftUI layout repositions it. Directly focusing the new terminal caused the first click to be swallowed by the stale portal geometry before it could be updated. Fix: when the split source is a BrowserPanel, focus the browser panel first (synchronously, so it participates in the current layout pass), then hand focus to the new terminal on the next runloop after SwiftUI has updated the portal geometry. This mirrors the behavior of the user manually clicking browser → terminal, which was confirmed to work. The previous TabManager workaround (isBrowserSource + DispatchQueue .main.async focusPanel) is removed; newTerminalSplit now handles the browser-source focus path directly and consistently. Only Workspace.swift and TabManager.swift are changed. BrowserPanel and BrowserWindowPortal are untouched. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
a7b1428 to
474f7bf
Compare
Phase 11: fix first-click swallowed after browser-panel terminal split (474f7bf)
Root causeWhen This only affects browser-only workspaces. When the source is a FixIn
if let browserSourcePanel {
// Focus the browser first so SwiftUI updates the portal geometry,
// then hand off to the terminal on the next runloop.
focusPanel(browserSourcePanel.id, previousHostedView: previousHostedView)
DispatchQueue.main.async { [weak self] in
guard let self else { return }
self.focusPanel(newPanel.id, previousHostedView: nil)
previousHostedView?.clearSuppressReparentFocus()
}
}This mirrors what the user doing manually — clicking the browser then clicking the terminal — which was confirmed to work correctly. Why not the previous approachThe prior commit (
Rather than patch those two issues, we reverted to Changes
Tested
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/Workspace.swift (1)
6615-6627:⚠️ Potential issue | 🟠 MajorKeep the browser-path reparent suppression balanced.
Line 6615 only suppresses
previousHostedView, so the sourceBrowserPanelcan still reacquire first responder during SwiftUI reparenting. Also, Line 6624 returns before the clear runs, so that suppression can stay latched if the workspace is gone by the next turn. Enter the browser suppressor here and clear both suppressors from adeferinside the async block.Suggested fix
if focus { previousHostedView?.suppressReparentFocus() if let browserSourcePanel { + browserSourcePanel.suppressReparentFocus() + let newPanelId = newPanel.id // When the source is a BrowserPanel, the WKWebView portal still covers the full // workspace rect until SwiftUI layout shrinks it. Focusing the terminal directly // causes the first click to be swallowed by the stale portal geometry. // Focusing the browser first lets the portal settle, then we hand off to the // terminal on the next runloop — mirroring the user clicking browser → terminal. focusPanel(browserSourcePanel.id, previousHostedView: previousHostedView) - DispatchQueue.main.async { [weak self] in - guard let self else { return } - self.focusPanel(newPanel.id, previousHostedView: nil) - previousHostedView?.clearSuppressReparentFocus() + DispatchQueue.main.async { [weak self, weak browserSourcePanel, weak previousHostedView] in + defer { + previousHostedView?.clearSuppressReparentFocus() + browserSourcePanel?.clearSuppressReparentFocus() + } + guard let self else { return } + self.focusPanel(newPanelId, previousHostedView: nil) } } else { focusPanel(newPanel.id, previousHostedView: previousHostedView) DispatchQueue.main.asyncAfter(deadline: .now() + 0.05) { previousHostedView?.clearSuppressReparentFocus()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 6615 - 6627, The suppression is unbalanced: call suppressReparentFocus() on the browser source as well as previousHostedView before calling focusPanel(browserSourcePanel.id...), then in the DispatchQueue.main.async closure use a defer to call clearSuppressReparentFocus() on both previousHostedView and browserSourcePanel (so both suppressions are always cleared even if self is nil or the method returns early); update the closure that currently calls focusPanel(newPanel.id, previousHostedView: nil) to first install the defer clearing both suppressors and then perform the handoff, referencing previousHostedView, browserSourcePanel, focusPanel(...), suppressReparentFocus and clearSuppressReparentFocus to locate the changes.Sources/TabManager.swift (1)
1181-1199:⚠️ Potential issue | 🟠 MajorReturn handled only when a terminal was actually created.
At Line 1181 and Line 1198, the method can return
trueeven whenpanelIdisnilornewTerminalSplitreturnsnil, which means the drop is marked handled but no shell opens.Proposed hardening
if let existing { selectedTabId = existing.id // Prefer a focused or existing TerminalPanel as the split source. // If the workspace has only non-terminal panels (e.g. browser-only), use any panel // so a new terminal appears as a split beside it rather than creating a new workspace. let panelId = existing.focusedPanelId ?? existing.panels.values.compactMap { $0 as? TerminalPanel }.first?.id ?? existing.panels.values.first?.id - if let panelId { - let newPanel = existing.newTerminalSplit(from: panelId, orientation: .horizontal) + if let panelId, + existing.newTerminalSplit(from: panelId, orientation: .horizontal) != nil { `#if` DEBUG - if newPanel == nil { - dlog("sidebar.folderDrop.splitFailed panelId=\(panelId.uuidString.prefix(5))") - } + dlog("sidebar.folderDrop.existing workspaceId=\(existing.id.uuidString.prefix(5))") `#endif` + } else { + _ = addWorkspace(workingDirectory: path, select: true) +#if DEBUG + dlog("sidebar.folderDrop.fallback.new path=\(path)") +#endif } -#if DEBUG - dlog("sidebar.folderDrop.existing workspaceId=\(existing.id.uuidString.prefix(5))") -#endif } else { addWorkspace(workingDirectory: path, select: true) `#if` DEBUG dlog("sidebar.folderDrop.new path=\(path)") `#endif` } return true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1181 - 1199, The drop handler currently returns true even when no terminal was created (e.g., panelId is nil or newTerminalSplit(from:orientation:) returns nil); change the control flow so the function only returns true when a terminal split was actually created or when addWorkspace(workingDirectory:select:) is successfully invoked—specifically, check that panelId is non-nil and newTerminalSplit(...) != nil before returning true in the existing-branch, and only return true after calling addWorkspace(...) in the else-branch (or propagate addWorkspace success) so the drop isn’t marked handled when nothing was created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/TabManager.swift`:
- Around line 1181-1199: The drop handler currently returns true even when no
terminal was created (e.g., panelId is nil or
newTerminalSplit(from:orientation:) returns nil); change the control flow so the
function only returns true when a terminal split was actually created or when
addWorkspace(workingDirectory:select:) is successfully invoked—specifically,
check that panelId is non-nil and newTerminalSplit(...) != nil before returning
true in the existing-branch, and only return true after calling
addWorkspace(...) in the else-branch (or propagate addWorkspace success) so the
drop isn’t marked handled when nothing was created.
In `@Sources/Workspace.swift`:
- Around line 6615-6627: The suppression is unbalanced: call
suppressReparentFocus() on the browser source as well as previousHostedView
before calling focusPanel(browserSourcePanel.id...), then in the
DispatchQueue.main.async closure use a defer to call
clearSuppressReparentFocus() on both previousHostedView and browserSourcePanel
(so both suppressions are always cleared even if self is nil or the method
returns early); update the closure that currently calls focusPanel(newPanel.id,
previousHostedView: nil) to first install the defer clearing both suppressors
and then perform the handoff, referencing previousHostedView,
browserSourcePanel, focusPanel(...), suppressReparentFocus and
clearSuppressReparentFocus to locate the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8279897e-2f66-44aa-b0ef-748f299d9cf3
📒 Files selected for processing (2)
Sources/TabManager.swiftSources/Workspace.swift
|
Superseded by #1821 — cleaner commit history and return-false fix included. |
Demo
demo.mov
Summary
Behavior
.app,.xcworkspace, etc.)Implementation
FileDropOverlayView.performDragOperation(ContentView.swift): when no terminal is under the cursor, reads URLs from the pasteboard and delegates toonDropFileDropOverlayView.updateDragTarget: returns.copyonly when the payload contains at least one plain-directory URL (hasDirectoryPath && !isPackage), keeping cursor affordance consistent with actual acceptanceTabManager.handleSidebarFolderDrop: filters to plain directories viaisDirectory + isPackageresource keys, normalises paths withFinderServicePathResolver.orderedUniqueDirectories(same pattern as the rest of the codebase), then selects+splits or creates a workspaceTesting
sidebar.folderDrop.existingin log).appbundle → nothing happens, no copy cursorRelated
Builds on #1571 (dock icon drag support).
Summary by cubic
Drag a folder from Finder onto the sidebar to open a terminal in that folder by selecting a matching workspace (adds a split) or creating a new one. If the match is browser‑only, a terminal is split beside it and focus is restored correctly.
New Features
Bug Fixes
Written for commit 474f7bf. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements