Fix fullscreen new windows opening in current Space - #2345
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAppDelegate.createMainWindow now separates resolution of Changes
Sequence Diagram(s)sequenceDiagram
participant AppDelegate
participant SourceWindow
participant NewWindow
participant MainQueue
AppDelegate->>SourceWindow: resolve sourceContext and sourceWindow
AppDelegate->>AppDelegate: derive existingFrame from sourceWindow?.frame
alt source is native fullscreen and no session snapshot
AppDelegate->>NewWindow: create NSWindow and add .fullScreenDisallowsTiling
else
AppDelegate->>NewWindow: create NSWindow without flag
end
AppDelegate->>NewWindow: showAndMakeKeyAndOrderFront / activate
AppDelegate->>MainQueue: DispatchQueue.main.async { remove .fullScreenDisallowsTiling }
MainQueue->>NewWindow: remove .fullScreenDisallowsTiling (if still present)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a bug where newly created main windows would join an existing native fullscreen Space instead of opening on a separate Desktop. The fix adds Key observations:
Confidence Score: 5/5Safe to merge; the fix is minimal and correct with no risk of regression. The only finding is a P2 process issue (single-commit instead of two-commit structure for the regression test). The production code change is a one-liner that correctly adds a flag, and the No files require special attention for correctness; the regression test commit structure in Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant NSWindow
User->>AppDelegate: Cmd+Shift+N (createMainWindow)
AppDelegate->>NSWindow: init(contentRect:styleMask:backing:defer:)
Note over NSWindow: Default collectionBehavior<br/>(e.g. .managed)
AppDelegate->>NSWindow: collectionBehavior.insert(.fullScreenDisallowsTiling)
Note over NSWindow: Now disallows joining<br/>existing fullscreen Space
AppDelegate->>NSWindow: applyWindowDecorations(to:)
Note over NSWindow: collectionBehavior unchanged<br/>(decorations only touch traffic lights)
AppDelegate->>NSWindow: orderFront / makeKeyAndOrderFront
NSWindow-->>User: New window opens on separate Space
Reviews (1): Last reviewed commit: "Fix fullscreen new windows opening in cu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func testCreateMainWindowDisallowsFullScreenTiling() { | ||
| guard let appDelegate = AppDelegate.shared else { | ||
| XCTFail("Expected AppDelegate.shared") | ||
| return | ||
| } | ||
|
|
||
| let windowId = appDelegate.createMainWindow() | ||
| defer { | ||
| closeWindow(withId: windowId) | ||
| } | ||
|
|
||
| guard let window = window(withId: windowId) else { | ||
| XCTFail("Expected test window") | ||
| return | ||
| } | ||
|
|
||
| XCTAssertTrue( | ||
| window.collectionBehavior.contains(.fullScreenDisallowsTiling), | ||
| "Main windows should opt out of fullscreen tiling so new windows do not join an existing fullscreen Space" | ||
| ) | ||
| } |
There was a problem hiding this comment.
Both the failing test and the fix are bundled in the same commit (cade637b). CLAUDE.md (and AGENTS.md) require a two-commit structure for regression tests so that CI can confirm the test actually fails without the fix:
Commit 1: Add the failing test only (no fix). CI should go red.
Commit 2: Add the fix. CI should go green.
As submitted, there's no way to verify in the PR's commit history that testCreateMainWindowDisallowsFullScreenTiling would have caught the bug, since the test and the fix land together. Please rebase into two commits so the Commits tab on GitHub shows a red check on commit 1 and green on commit 2.
Context Used: CLAUDE.md (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
102-122: Make the non-fullscreen regression test deterministic.This test currently depends on ambient window state. If the resolved source window is fullscreen, the assertion can fail for environment reasons rather than a real regression. Force the source condition explicitly in-test.
Proposed tweak
func testCreateMainWindowDoesNotDisallowFullScreenTilingByDefault() { guard let appDelegate = AppDelegate.shared else { XCTFail("Expected AppDelegate.shared") return } + appDelegate.debugCreateMainWindowSourceIsNativeFullScreenOverride = false + defer { + appDelegate.debugCreateMainWindowSourceIsNativeFullScreenOverride = nil + } let windowId = appDelegate.createMainWindow() defer { closeWindow(withId: windowId) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 102 - 122, The test can flake if an ambient window is fullscreen—make it deterministic by explicitly forcing a non-fullscreen source before calling createMainWindow(): obtain AppDelegate.shared (or NSApp.windows) and ensure any candidate source window is not fullscreen (exit fullscreen or clear full-screen collectionBehavior) or create a new dummy non-fullscreen window and use that as the resolved source, then call createMainWindow(), proceed with window(withId:) and the existing assertions; target symbols: AppDelegate.shared, createMainWindow(), window(withId:), closeWindow(withId:).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 102-122: The test can flake if an ambient window is
fullscreen—make it deterministic by explicitly forcing a non-fullscreen source
before calling createMainWindow(): obtain AppDelegate.shared (or NSApp.windows)
and ensure any candidate source window is not fullscreen (exit fullscreen or
clear full-screen collectionBehavior) or create a new dummy non-fullscreen
window and use that as the resolved source, then call createMainWindow(),
proceed with window(withId:) and the existing assertions; target symbols:
AppDelegate.shared, createMainWindow(), window(withId:), closeWindow(withId:).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e2e6803-f68f-424e-ad32-514d8fa0de68
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
* Fix fullscreen new windows opening in current Space * works * Stabilize fullscreen tiling regression test
This branch was successfully deployed
1 participant
Summary
Testing
Fixes #2315
Summary by cubic
New main windows created from a native fullscreen window no longer join that Space. We temporarily disable fullscreen tiling during creation (when not restoring a session) so they open on a separate Space, then restore normal Split View support. Fixes #2315.
.fullScreenDisallowsTilingonly when opened from a native fullscreen source and no session restore is in progress; remove it on the next run loop tick after the window appears.Written for commit ab47673. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests