Repository navigation
Keep Cmd-Shift-N windows on source display - #3214
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThreads a preferred source NSWindow through main-window creation: menu/shortcut/event context resolves a source window, which is passed into createMainWindow and used to position new windows with cascade and screen-visible-frame clamping. Changes
Sequence DiagramsequenceDiagram
participant User as User (shortcut/menu)
participant App as AppDelegate
participant EventCtx as Event / Sender Resolution
participant Creator as createMainWindow
participant Position as positionNewMainWindow
User->>App: trigger new-window action (with event/sender)
App->>EventCtx: preferredSourceWindowForNewMainWindow(sender)
EventCtx-->>App: resolved preferredSourceWindow
App->>Creator: createMainWindow(..., sourceWindow: preferredSourceWindow)
Creator->>Creator: resolvedMainWindowSource(preferredSourceWindow)
Creator->>Position: positionNewMainWindow(resolved source)
Position-->>Creator: positioned frame (clamped, cascaded)
Creator-->>App: new main window UUID / instance
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryRoutes Cmd-Shift-N through the shortcut event's source window so new windows open on the same display as the triggering cmux window, using a 24-pt cascade offset clamped to the source screen's visible frame. The command-palette path is updated to scope the preferred window to the current Confidence Score: 4/5Safe to merge; only P2 style findings, logic and test structure are correct All findings are P2. The two-commit test structure is followed per policy, the resolution chain handles nil and non-terminal windows gracefully, and clamping prevents off-screen placement. Minor concerns about minWidth/minHeight floor and double singleton dereference do not affect correctness. Sources/AppDelegate.swift — clampFrame minWidth/minHeight floor in positionNewMainWindow Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Cmd-Shift-N shortcut event] --> B[mainWindowForShortcutEvent]
B --> C{resolvedShortcutEventWindow\nisMainTerminalWindow?}
C -- yes --> D[sourceWindow = event window]
C -- no --> E{NSApp.keyWindow\nisMainTerminalWindow?}
E -- yes --> D
E -- no --> F{NSApp.mainWindow\nisMainTerminalWindow?}
F -- yes --> D
F -- no --> G[sourceWindow = nil]
D --> H[createMainWindow sourceWindow: window]
G --> H
H --> I[resolvedMainWindowSource]
I --> J{preferredSourceWindow is terminal window?}
J -- yes --> K[use as source]
J -- no --> L{contextForMainWindow or contextForMainTerminalWindow?}
L -- found --> M[resolvedWindow for context]
L -- not found --> N[fall back to preferredMainWindowContextForWorkspaceCreation]
K --> O[positionNewMainWindow]
M --> O
N --> P{sourceWindow nil?}
P -- yes --> Q[window.center + cascadeTopLeft]
P -- no --> O
O --> R[cascade 24pt from source origin clampFrame to source screen visibleFrame]
R --> S[window displayed on source display]
PA[palette.newWindow] --> PB[mainWindow for windowId] --> H
Reviews (1): Last reviewed commit: "Place Cmd-Shift-N windows on source disp..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Self.clampFrame(frame, within: visibleFrame, minWidth: 1, minHeight: 1), | ||
| display: false | ||
| ) |
There was a problem hiding this comment.
clampFrame is called with minWidth: 1, minHeight: 1. If the source window is so large that it nearly fills the display and the 24-pt cascade pushes the new window outside the visible frame, the clamp will shrink it down to as small as 1×1 pt — effectively invisible and non-resizable in practice. Other call sites in the file typically pass something closer to the minimum usable size. Consider using a sensible minimum (e.g., the default 460×360 fallback rect) so the window remains functional after clamping.
There was a problem hiding this comment.
Fixed by using the app's default minimum window size when clamping the cascaded frame, so the new window remains usable on constrained displays.
— Claude Code
| registry.register(commandId: "palette.newWindow") { | ||
| AppDelegate.shared?.openNewMainWindow(nil) | ||
| AppDelegate.shared?.openNewMainWindow( | ||
| preferredWindow: AppDelegate.shared?.mainWindow(for: windowId) | ||
| ) | ||
| } |
There was a problem hiding this comment.
AppDelegate.shared is dereferenced twice: once for the call site and again inside the argument expression. Because AppDelegate.shared is a singleton the double dereference is safe, but it reads as if the inner call could diverge. Capturing the delegate once is cleaner:
registry.register(commandId: "palette.newWindow") {
guard let delegate = AppDelegate.shared else { return }
delegate.openNewMainWindow(preferredWindow: delegate.mainWindow(for: windowId))
}Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed by capturing AppDelegate.shared once in the palette command and using that delegate for both the lookup and the new-window call.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/AppDelegate.swift (1)
6205-6241: Use shared minimum-size policy instead of hardcoded values.
Line 6226introduces a second source of truth (460x360). PreferSessionPersistencePolicy.minimumWindowWidth/Heighthere to keep clamping behavior consistent across restore/create paths.♻️ Suggested change
- let minimumWindowSize = NSSize(width: 460, height: 360) + let minimumWindowSize = NSSize( + width: CGFloat(SessionPersistencePolicy.minimumWindowWidth), + height: CGFloat(SessionPersistencePolicy.minimumWindowHeight) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 6205 - 6241, The clamp in positionNewMainWindow currently uses a hardcoded minimumWindowSize (460x360); replace that with the shared policy values by using SessionPersistencePolicy.minimumWindowWidth and SessionPersistencePolicy.minimumWindowHeight when calling Self.clampFrame so the same minimum-size policy is used across restore/create paths; update/remove the local minimumWindowSize variable and ensure the clampFrame call passes those two policy values instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 6205-6241: The clamp in positionNewMainWindow currently uses a
hardcoded minimumWindowSize (460x360); replace that with the shared policy
values by using SessionPersistencePolicy.minimumWindowWidth and
SessionPersistencePolicy.minimumWindowHeight when calling Self.clampFrame so the
same minimum-size policy is used across restore/create paths; update/remove the
local minimumWindowSize variable and ensure the clampFrame call passes those two
policy values instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71aa3a54-82be-48a2-acec-a033941a398c
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/ContentView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/ContentView.swift
This branch was successfully deployed
1 participant
Summary
Testing
./scripts/reload.sh --tag cmdwinpassed.Issues
Summary by cubic
Cmd+Shift+N now opens a new window on the same display as the window that triggered it, placed next to it and matching its size, instead of creating a workspace elsewhere. The command palette “New Window” and shortcut now explicitly target the current cmux window. Addresses Linear task: Cmd-Shift-N should create a new window on the same display, not a new workspace.
Written for commit 40f4b4b. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Tests