Repository navigation
Fix session restore suppression on relaunch - #2469
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe changes enhance AppDelegate and FinderServicePathResolver to exclude bundle-relative paths from being opened as explicit directories, preventing LaunchServices-provided self paths from triggering folder-open handling or suppressing session restore on relaunch. Changes
Sequence Diagram(s)sequenceDiagram
participant NSApp as NSApplication
participant AppDel as AppDelegate
participant FSResolver as FinderServicePathResolver
participant URLComp as URL Comparison
NSApp->>AppDel: application(_:open:)
AppDel->>FSResolver: orderedUniqueDirectories(from:excludingDescendantsOf:)
FSResolver->>URLComp: normalizeAndCompare each URL
URLComp->>URLComp: standardizedFileURL + resolvingSymlinksInPath
alt Is descendant of excluded root?
URLComp-->>FSResolver: skip (return nil)
else Non-descendant path
URLComp-->>FSResolver: include in results
end
FSResolver-->>AppDel: filtered [String] directories
AppDel->>AppDel: prepareForExplicitOpenIntentAtStartup()
AppDel-->>NSApp: open workspaces/windows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 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 |
![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.
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 `@cmuxTests/WindowAndDragTests.swift`:
- Around line 233-264: The test testApplicationOpenURLsIgnoresBundleSelfPaths
can falsely pass if first-run onboarding suppresses application(_:open:), so
before creating AppDelegate add the same onboarding/welcome-key setup used in
the dropped-folder test to mark onboarding as completed (i.e., set the same
UserDefaults key the dropped-folder test sets), placing that UserDefaults write
before instantiating app = AppDelegate() so application(_:open:) is exercised
normally and the bundle-self filtering logic in application(_:open:) is actually
tested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e58d057-3ca4-45d0-855d-0aa251650257
📒 Files selected for processing (3)
Sources/AppDelegate.swiftcmuxTests/OmnibarAndToolsTests.swiftcmuxTests/WindowAndDragTests.swift
| func testApplicationOpenURLsIgnoresBundleSelfPaths() { | ||
| _ = NSApplication.shared | ||
| let app = AppDelegate() | ||
|
|
||
| let windowId = UUID() | ||
| let window = makeMainWindow(id: windowId) | ||
| defer { window.orderOut(nil) } | ||
|
|
||
| let manager = TabManager() | ||
| app.registerMainWindow( | ||
| window, | ||
| windowId: windowId, | ||
| tabManager: manager, | ||
| sidebarState: SidebarState(), | ||
| sidebarSelectionState: SidebarSelectionState() | ||
| ) | ||
|
|
||
| window.makeKeyAndOrderFront(nil) | ||
| _ = app.synchronizeActiveMainWindowContext(preferredWindow: window) | ||
|
|
||
| let existingWorkspaceIds = Set(manager.tabs.map(\.id)) | ||
| let embeddedExecutableURL = Bundle.main.bundleURL | ||
| .appendingPathComponent("Contents/MacOS/cmux", isDirectory: false) | ||
|
|
||
| app.application( | ||
| NSApplication.shared, | ||
| open: [embeddedExecutableURL] | ||
| ) | ||
|
|
||
| let createdWorkspace = manager.tabs.first { !existingWorkspaceIds.contains($0.id) } | ||
| XCTAssertNil(createdWorkspace) | ||
| } |
There was a problem hiding this comment.
Stabilize onboarding preconditions so this regression can’t pass for the wrong reason.
If first-run onboarding suppresses application(_:open:) routing, this test can pass even when bundle-self filtering regresses. Mirror the welcome-key setup used in the dropped-folder test to isolate the intended behavior.
Suggested patch
func testApplicationOpenURLsIgnoresBundleSelfPaths() {
_ = NSApplication.shared
let app = AppDelegate()
@@
window.makeKeyAndOrderFront(nil)
_ = app.synchronizeActiveMainWindowContext(preferredWindow: window)
+ let defaults = UserDefaults.standard
+ let previousWelcomeShown = defaults.object(forKey: WelcomeSettings.shownKey)
+ defaults.set(true, forKey: WelcomeSettings.shownKey)
+ defer {
+ if let previousWelcomeShown {
+ defaults.set(previousWelcomeShown, forKey: WelcomeSettings.shownKey)
+ } else {
+ defaults.removeObject(forKey: WelcomeSettings.shownKey)
+ }
+ }
+
let existingWorkspaceIds = Set(manager.tabs.map(\.id))
let embeddedExecutableURL = Bundle.main.bundleURL
.appendingPathComponent("Contents/MacOS/cmux", isDirectory: false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/WindowAndDragTests.swift` around lines 233 - 264, The test
testApplicationOpenURLsIgnoresBundleSelfPaths can falsely pass if first-run
onboarding suppresses application(_:open:), so before creating AppDelegate add
the same onboarding/welcome-key setup used in the dropped-folder test to mark
onboarding as completed (i.e., set the same UserDefaults key the dropped-folder
test sets), placing that UserDefaults write before instantiating app =
AppDelegate() so application(_:open:) is exercised normally and the bundle-self
filtering logic in application(_:open:) is actually tested.
Greptile SummaryThis PR fixes a session-restore suppression bug on relaunch: macOS LaunchServices was passing the running app bundle's URL to Key changes:
Confidence Score: 5/5Safe to merge — fix is logically correct, follows the two-commit policy, and is well-covered by unit and integration tests. All remaining findings are P2. No logic errors, data-loss risk, or broken contracts found. The core fix matches the pre-existing pattern in application(_:open:). cmuxTests/WindowAndDragTests.swift — missing welcome-screen suppression boilerplate that could cause a false failure on a fresh CI runner. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["application(_:open: urls)"] --> B["externalOpenDirectories(from: urls)"]
B --> C["Filter: isFileURL only"]
C --> D["orderedUniqueDirectories(excludingDescendantsOf: [Bundle.main.bundleURL])"]
D --> E{"For each URL"}
E --> F["resolvedDirectoryURL(url)\n(standardizedFileURL only, symlinks preserved)"]
F --> G{"isSameOrDescendant(directoryURL, of: bundleURL)?"}
G -- "normalizedComparisonURL\n(standardize + resolvingSymlinks) on BOTH sides" --> H{"Prefix match of path components?"}
H -- "Yes: bundle path" --> I["skip / continue"]
H -- "No: external path" --> J["canonicalDirectoryPath (symlink alias preserved)"]
J --> K["Append to directories"]
I --> E
K --> E
E -- done --> L{"directories.isEmpty?"}
L -- "Yes" --> M["return early — prepareForExplicitOpenIntentAtStartup NOT called → session restore unsuppressed"]
L -- "No" --> N["prepareForExplicitOpenIntentAtStartup() then open workspace for each dir"]
Reviews (1): Last reviewed commit: "Preserve symlink aliases for external op..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private static func normalizedComparisonURL(_ url: URL) -> URL { | ||
| url.standardizedFileURL.resolvingSymlinksInPath() | ||
| } | ||
|
|
||
| private static func isSameOrDescendant(_ url: URL, of rootURL: URL) -> Bool { | ||
| let urlPathComponents = normalizedComparisonURL(url).pathComponents | ||
| let rootPathComponents = normalizedComparisonURL(rootURL).pathComponents | ||
| guard urlPathComponents.count >= rootPathComponents.count else { return false } | ||
| return Array(urlPathComponents.prefix(rootPathComponents.count)) == rootPathComponents | ||
| } |
There was a problem hiding this comment.
normalizedComparisonURL for excluded root recomputed per-input-URL
isSameOrDescendant(_:of:) is called inside an inner closure (excludedRootURLs.contains(where:)), so normalizedComparisonURL(rootURL) — which calls resolvingSymlinksInPath() (a synchronous filesystem stat chain) — is re-executed on the same excludedRootURL once for every URL in the input array.
For the typical launch-time use (excludedRootURLs = [Bundle.main.bundleURL], a handful of URLs), the overhead is negligible. But the current shape makes it easy for a future caller to pass a large pathURLs array and unknowingly amplify the I/O. Pre-computing the normalised roots before the loop would eliminate the redundancy at zero semantic cost.
| func testApplicationOpenURLsIgnoresBundleSelfPaths() { | ||
| _ = NSApplication.shared | ||
| let app = AppDelegate() | ||
|
|
||
| let windowId = UUID() | ||
| let window = makeMainWindow(id: windowId) | ||
| defer { window.orderOut(nil) } | ||
|
|
||
| let manager = TabManager() | ||
| app.registerMainWindow( | ||
| window, | ||
| windowId: windowId, | ||
| tabManager: manager, | ||
| sidebarState: SidebarState(), | ||
| sidebarSelectionState: SidebarSelectionState() | ||
| ) | ||
|
|
||
| window.makeKeyAndOrderFront(nil) | ||
| _ = app.synchronizeActiveMainWindowContext(preferredWindow: window) | ||
|
|
||
| let existingWorkspaceIds = Set(manager.tabs.map(\.id)) | ||
| let embeddedExecutableURL = Bundle.main.bundleURL | ||
| .appendingPathComponent("Contents/MacOS/cmux", isDirectory: false) | ||
|
|
||
| app.application( | ||
| NSApplication.shared, | ||
| open: [embeddedExecutableURL] | ||
| ) | ||
|
|
||
| let createdWorkspace = manager.tabs.first { !existingWorkspaceIds.contains($0.id) } | ||
| XCTAssertNil(createdWorkspace) | ||
| } |
There was a problem hiding this comment.
The existing testApplicationOpenURLsCreatesWorkspaceForDroppedDirectory test (lines 184–231 just above) explicitly suppresses the welcome-tab by writing to UserDefaults.standard before calling app.application(_:open:), then restores it in a defer. That guard was presumably added because, in a fresh CI environment where the welcome screen has never been shown, some AppDelegate init path can add an extra tab after the existingWorkspaceIds snapshot.
This new test skips that guard. Since the snapshot is captured before the call under test, any welcome-generated tab that appears after the snapshot would cause a false XCTAssertNil failure. Adding the same UserDefaults setup-and-restore block (before the existingWorkspaceIds snapshot) would make this test consistent with the established pattern and eliminate that potential flake.
This branch was successfully deployed
1 participant
Summary
Verification
Summary by cubic
Fixes session restore being suppressed on relaunch by ignoring self-bundle URLs surfaced by LaunchServices. Preserves symlink alias paths while still excluding bundle paths. (Addresses issue-2387.)
Written for commit 8258459. Summary will update on new commits.
Summary by CodeRabbit