Repository navigation
Route main window bootstrap through AppDelegate - #3164
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-main-window FileExplorerState wiring into MainWindowContext and registration, disables AppKit window restoration for main windows, hardens registerMainWindow against duplicate windowId registrations (detects/closes duplicates), and implements reopen/bootstrap logic to ensure and activate an initial main window. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NSApp as NSApplication
participant AppDel as AppDelegate
participant Window as MainWindow (NSWindow)
participant Context as MainWindowContext
User->>NSApp: open or reopen app
NSApp->>AppDel: applicationShouldHandleReopen(hasVisibleWindows)
AppDel->>AppDel: ensureInitialMainWindowIfNeeded(shouldActivate)
AppDel->>Window: create/activate main window (set isRestorable = false)
Window->>AppDel: onAppear -> registerMainWindow(window, windowId, fileExplorerState...)
AppDel->>Context: attach FileExplorerState to MainWindowContext
AppDel->>AppDel: check for existing visible window with same windowId
alt duplicate found
AppDel->>Window: orderOut() / close()
AppDel-->>NSApp: skip registration
else unique
AppDel->>AppDel: store/sync MainWindowContext and activate window
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
3290-3314:⚠️ Potential issue | 🟠 MajorDon't retarget
tabManager.windowbefore rejecting the duplicate.Line 3290 assigns
tabManager.window = windowbefore the duplicate-window guard. In the duplicate-restoration path, this method closeswindowand returns, so the sharedTabManageris left pointing at the just-closed duplicate instead of the surviving main window.Suggested fix
- tabManager.window = window - let key = ObjectIdentifier(window) `#if` DEBUG let priorManagerToken = debugManagerToken(self.tabManager) `#endif` if let existing = mainWindowContexts[key] { + tabManager.window = window existing.window = window if let cmuxConfigStore { existing.cmuxConfigStore = cmuxConfigStore } } else if let existing = mainWindowContexts.values.first(where: { $0.windowId == windowId }) { @@ window.orderOut(nil) window.close() return } + tabManager.window = window existing.window = window if let cmuxConfigStore { existing.cmuxConfigStore = cmuxConfigStore } reindexMainWindowContextIfNeeded(existing, for: window) } else { + tabManager.window = window mainWindowContexts[key] = MainWindowContext( windowId: windowId, tabManager: tabManager, sidebarState: sidebarState,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 3290 - 3314, The code assigns tabManager.window = window before checking for a duplicate main window, which leaves TabManager pointing at a closed duplicate when the early-return duplicate branch runs; fix by deferring or correcting that assignment: move the tabManager.window = window assignment to after the duplicate detection/return block or, if you prefer minimal change, set tabManager.window = existing.window in the duplicate branch before window.orderOut(nil)/window.close()/return so the TabManager always references the surviving window (refer to tabManager.window, mainWindowContexts, windowId, and the duplicate-detection block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 3290-3314: The code assigns tabManager.window = window before
checking for a duplicate main window, which leaves TabManager pointing at a
closed duplicate when the early-return duplicate branch runs; fix by deferring
or correcting that assignment: move the tabManager.window = window assignment to
after the duplicate detection/return block or, if you prefer minimal change, set
tabManager.window = existing.window in the duplicate branch before
window.orderOut(nil)/window.close()/return so the TabManager always references
the surviving window (refer to tabManager.window, mainWindowContexts, windowId,
and the duplicate-detection block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c0ab80a-e90f-4ed6-9b25-5fe241120900
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/ContentView.swift
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4913d171be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| window.orderOut(nil) | ||
| window.close() | ||
| return |
There was a problem hiding this comment.
When this duplicate-window branch closes and returns, registerMainWindow has already assigned tabManager.window to the duplicate at function entry, so in the exact restore scenario this patch targets (two windows sharing one app model) the TabManager can be left pointing at a closing/deallocated window. That breaks subsequent title routing in TabManager.updateWindowTitle(for:) until the surviving window re-registers, which may not happen immediately. Rebind the manager to existingWindow (or defer assignment until after duplicate filtering) before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ae9bdb1. registerMainWindow now assigns tabManager.window only after accepting the incoming window, and the duplicate-reject branch keeps the existing manager bound to the surviving window.
— Claude Code
Greptile SummaryThis PR guards against duplicate main windows created by SwiftUI/AppKit restoration by (a) setting
Confidence Score: 3/5Hold for the tabManager mutation ordering fix before merging; the duplicate-close itself is sound but the unconditional pre-guard assignment risks a dangling-window reference on the surviving window's TabManager. One P1 finding: tabManager.window is set to the duplicate before the early-return guard can fire, potentially leaving the live window's TabManager holding a reference to the just-closed NSWindow if the two windows share the same model instance (as described in the PR). No P0 issues. ContentView change is clean. Sources/AppDelegate.swift — specifically the ordering of Important Files Changed
Sequence DiagramsequenceDiagram
participant OS as AppKit/SwiftUI Restore
participant CW as ContentView (WindowAccessor)
participant AD as AppDelegate.registerMainWindow
participant MW as mainWindowContexts
OS->>CW: restore duplicate window (same windowId)
CW->>CW: window.isRestorable = false
CW->>AD: registerMainWindow(dupWindow, windowId, tabManager)
AD->>AD: tabManager.window = dupWindow ⚠️ (unconditional)
AD->>MW: lookup existing context for windowId
MW-->>AD: existing context found (existingWindow is visible)
AD->>AD: dupWindow.orderOut(nil)
AD->>AD: dupWindow.close()
AD-->>CW: return (early)
Note over AD,MW: tabManager.window still points to closed dupWindow
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
You’re at about 98% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
lawrencecchen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/AppDelegate.swift (1)
3299-3325:⚠️ Potential issue | 🟠 MajorRun duplicate-window filtering before rebinding the shared
TabManager.window.Line 3299 updates
tabManager.windowbefore the duplicate check runs. In the restore case this PR is fixing, the duplicate can share the sameTabManager, so the soon-to-close window still steals that pointer even though registration returns early. TheexistingWindow.isVisibleguard also misses live miniaturized/original windows, which reopens the duplicate-state bug whenever the survivor is alive but hidden.Suggested fix
- tabManager.window = window - let key = ObjectIdentifier(window) `#if` DEBUG let priorManagerToken = debugManagerToken(self.tabManager) `#endif` if let existing = mainWindowContexts[key] { @@ - } else if let existing = mainWindowContexts.values.first(where: { $0.windowId == windowId }) { + } else if let existing = mainWindowContexts.values.first(where: { $0.windowId == windowId }) { if let existingWindow = existing.window, - existingWindow !== window, - existingWindow.isVisible { + existingWindow !== window { `#if` DEBUG cmuxDebugLog( "mainWindow.register.duplicateIgnored windowId=\(String(windowId.uuidString.prefix(8))) " + "existing={\(debugWindowToken(existingWindow))} duplicate={\(debugWindowToken(window))}" ) `#endif` window.orderOut(nil) window.close() return } @@ } + tabManager.window = window🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 3299 - 3325, Move the tabManager.window = window assignment so it happens after duplicate-window detection/registration completes; do not rebind the shared TabManager before you check for duplicates. In the duplicate-check branch that currently inspects existing.window and uses existingWindow.isVisible, change the visibility test to consider miniaturized windows as "alive" as well (e.g., treat existingWindow.isVisible || existingWindow.isMiniaturized as the live check) so a hidden/miniaturized survivor won't be treated as a duplicate that should be closed. Update logic around mainWindowContexts lookup (the branch that finds existing by windowId) to perform the duplicate-ignore/close and return before assigning tabManager.window or mutating the shared context fields.Sources/cmuxApp.swift (1)
189-203:⚠️ Potential issue | 🟠 MajorDon't attach long-lived setting observers to the self-closing bootstrap scene.
appearanceModeandsocketControlModeare now observed fromMainWindowBootstrapView, but that view closes its host window immediately. Once startup finishes, those.onChangehandlers are gone, so changing theme or socket mode later stops callingapplyAppearance()/updateSocketController()until the app is relaunched.Also applies to: 1021-1032
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 189 - 203, The .onChange handlers for appearanceMode and socketControlMode are attached to MainWindowBootstrapView which immediately closes, so applyAppearance() and updateSocketController() stop being called; move those observers out of MainWindowBootstrapView into a long-lived host (e.g., the App root view) or the same persistent object that calls bootstrapMainWindowScene(), and attach the change handlers there (or subscribe to the published properties from an App-level controller) so appearanceMode and socketControlMode changes always invoke applyAppearance() and updateSocketController().
🤖 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`:
- Around line 5083-5095: The reopen path in ensureInitialMainWindowIfNeeded
currently calls window.makeKeyAndOrderFront(nil) which doesn't reliably restore
a miniaturized window; modify the logic in ensureInitialMainWindowIfNeeded (for
the resolvedWindow(for:) branch) to call window.deminiaturize(nil) when
window.isMiniaturized (or just always call deminiaturize) before calling
window.makeKeyAndOrderFront(nil) and setActiveMainWindow(window), so the reused
window is unminimized when reopening instead of remaining hidden; keep
createMainWindow untouched.
- Around line 11719-11720: When tearing down an active window, also keep
AppDelegate.fileExplorerState in sync: update both unregisterMainWindow(_:) and
discardOrphanedMainWindowContext(_:) to mirror how you set fileExplorerState on
activation (e.g., set or clear AppDelegate.fileExplorerState alongside
repointing/clearing tabManager, sidebarState, and sidebarSelectionState). In
other words, when a main window context is being unregistered or discarded,
ensure you clear or repoint fileExplorerState (and any related file-explorer
selection state) so the file-explorer shortcut won’t target a stale or nil
state.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 3299-3325: Move the tabManager.window = window assignment so it
happens after duplicate-window detection/registration completes; do not rebind
the shared TabManager before you check for duplicates. In the duplicate-check
branch that currently inspects existing.window and uses
existingWindow.isVisible, change the visibility test to consider miniaturized
windows as "alive" as well (e.g., treat existingWindow.isVisible ||
existingWindow.isMiniaturized as the live check) so a hidden/miniaturized
survivor won't be treated as a duplicate that should be closed. Update logic
around mainWindowContexts lookup (the branch that finds existing by windowId) to
perform the duplicate-ignore/close and return before assigning tabManager.window
or mutating the shared context fields.
In `@Sources/cmuxApp.swift`:
- Around line 189-203: The .onChange handlers for appearanceMode and
socketControlMode are attached to MainWindowBootstrapView which immediately
closes, so applyAppearance() and updateSocketController() stop being called;
move those observers out of MainWindowBootstrapView into a long-lived host
(e.g., the App root view) or the same persistent object that calls
bootstrapMainWindowScene(), and attach the change handlers there (or subscribe
to the published properties from an App-level controller) so appearanceMode and
socketControlMode changes always invoke applyAppearance() and
updateSocketController().
🪄 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: b7e41b96-c725-4002-b8ce-989f50dd8eb9
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/ContentView.swiftSources/cmuxApp.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/ContentView.swift
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae9bdb1d88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .onAppear { | ||
| #if DEBUG | ||
| if ProcessInfo.processInfo.environment["CMUX_UI_TEST_MODE"] == "1" { | ||
| UpdateLogStore.shared.append("ui test: cmuxApp onAppear") | ||
| } | ||
| #endif | ||
| // Start the Unix socket controller for programmatic access | ||
| updateSocketController() | ||
| appDelegate.configure(tabManager: tabManager, notificationStore: notificationStore, sidebarState: sidebarState) | ||
| appDelegate.fileExplorerState = fileExplorerState | ||
| cmuxConfigStore.wireDirectoryTracking(tabManager: tabManager) | ||
| cmuxConfigStore.loadAll() | ||
| applyAppearance() | ||
| if ProcessInfo.processInfo.environment["CMUX_UI_TEST_SHOW_SETTINGS"] == "1" { | ||
| DispatchQueue.main.async { | ||
| appDelegate.openPreferencesWindow(debugSource: "uiTestShowSettings") | ||
| } | ||
| } | ||
| bootstrapMainWindowScene() | ||
| } |
There was a problem hiding this comment.
This change makes initial main-window creation depend entirely on WindowGroup’s .onAppear (bootstrapMainWindowScene()), but the same startup path already acknowledges that SwiftUI appearance callbacks can be delayed or skipped in UI-test environments. In that case the app never creates a real ContentView window, while configure has already wired AppDelegate to the placeholder state object, so automation/socket traffic can target detached state until a manual reopen occurs. Move the initial-window bootstrap to an AppDelegate launch hook so startup does not depend on view lifecycle timing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9d9a250. AppDelegate now bootstraps the first real main window during applicationDidFinishLaunching, and SwiftUI's hidden bootstrap view only re-enters the same idempotent path if it appears later.
— Claude Code
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
4926-4946:⚠️ Potential issue | 🟠 MajorInclude
fileExplorerStatein the active-context equality guard.Both sync helpers can now return early when
tabManagerand the sidebar stores already match, even iffileExplorerStatestill points at another window. That leaves the file explorer shortcut routed to stale state after any drift that only affects the explorer store.Suggested fix
let alreadyActive = tabManager === context.tabManager && sidebarState === context.sidebarState && sidebarSelectionState === context.sidebarSelectionState + && fileExplorerState === context.fileExplorerStateAlso applies to: 5726-5740
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 4926 - 4946, The early-return equality check named alreadyActive should include fileExplorerState so we do not treat contexts as identical when the file explorer store differs; update the guard that computes alreadyActive (which currently compares tabManager, sidebarState, and sidebarSelectionState against context.tabManager, context.sidebarState, and context.sidebarSelectionState) to also compare fileExplorerState == context.fileExplorerState, and apply the same change in the other identical site (the second helper around the block referencing setActiveMainWindow/windowForMainWindowId/TerminalController.shared) so both sync helpers only return early when the explorer state matches too.
♻️ Duplicate comments (2)
Sources/AppDelegate.swift (2)
5085-5096:⚠️ Potential issue | 🟠 MajorDeminiaturize reused windows during reopen.
applicationShouldHandleReopen(..., hasVisibleWindows: false)now funnels throughensureInitialMainWindowIfNeeded, butmakeKeyAndOrderFront(nil)does not reliably restore a miniaturized window. Reopening from the Dock can still leave the app with no visible main window.Suggested fix
for context in sortedMainWindowContextsForSessionSnapshot() { guard let window = resolvedWindow(for: context) else { continue } if shouldActivate { + if window.isMiniaturized { + window.deminiaturize(nil) + } window.makeKeyAndOrderFront(nil) setActiveMainWindow(window) } return context.windowId }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 5085 - 5096, When reopening we must deminiaturize a reused window because makeKeyAndOrderFront(nil) doesn't always restore a miniaturized window; update ensureInitialMainWindowIfNeeded to call window.deminiaturize(nil) on the resolvedWindow (before makeKeyAndOrderFront(nil)), then proceed to makeKeyAndOrderFront(nil) and setActiveMainWindow(window); keep the existing fallback to createMainWindow(shouldActivate:) when no context/window is found.
4680-4708:⚠️ Potential issue | 🟠 MajorRepoint or clear
fileExplorerStatewhen the active window is removed.These teardown paths still migrate
tabManager,sidebarState, andsidebarSelectionStateonly. Closing the active window can leaveAppDelegate.fileExplorerStatepointing at the dead window, and the new early-return guards above mean a later sync may not repair it anymore.Suggested fix
if tabManager === context.tabManager { if let nextContext = mainWindowContexts.values.first(where: { resolvedWindow(for: $0) != nil }) { tabManager = nextContext.tabManager sidebarState = nextContext.sidebarState sidebarSelectionState = nextContext.sidebarSelectionState + fileExplorerState = nextContext.fileExplorerState TerminalController.shared.setActiveTabManager(nextContext.tabManager) } else { tabManager = nil sidebarState = nil sidebarSelectionState = nil + fileExplorerState = nil TerminalController.shared.setActiveTabManager(nil) } }Apply the same change in both
discardOrphanedMainWindowContext(_:)andunregisterMainWindow(_:).Also applies to: 11755-11774
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 4680 - 4708, When removing an active MainWindowContext you must also repoint or clear AppDelegate.fileExplorerState like you do for tabManager, sidebarState and sidebarSelectionState to avoid it referencing a deallocated window; in discardOrphanedMainWindowContext(_:) (and similarly in unregisterMainWindow(_:)) after selecting nextContext (the one found via mainWindowContexts.values.first(where: { resolvedWindow(for: $0) != nil })) set fileExplorerState = nextContext.fileExplorerState (and when there is no nextContext set fileExplorerState = nil), ensuring this update happens alongside the existing tabManager/sidebar updates and TerminalController.shared.setActiveTabManager calls so an early-return later won’t leave fileExplorerState pointing at the dead window.
🤖 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`:
- Around line 3313-3326: The duplicate-registration guard only treats the
existing window as canonical when existingWindow.isVisible is true, so minimized
(miniaturized) main windows get ignored and duplicates are created; update the
conditional in the main-window registration branch to include miniaturized
windows (e.g., replace the existingWindow.isVisible check with a check that
treats existingWindow.isVisible OR existingWindow.isMiniaturized as canonical)
so the branch that sets existing.tabManager.window, calls window.orderOut(nil)
and window.close() still runs for minimized originals; keep the debug log calls
(cmuxDebugLog and debugWindowToken) and the same actions
(existing.tabManager.window = existingWindow, window.orderOut(nil),
window.close(), return).
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 4926-4946: The early-return equality check named alreadyActive
should include fileExplorerState so we do not treat contexts as identical when
the file explorer store differs; update the guard that computes alreadyActive
(which currently compares tabManager, sidebarState, and sidebarSelectionState
against context.tabManager, context.sidebarState, and
context.sidebarSelectionState) to also compare fileExplorerState ==
context.fileExplorerState, and apply the same change in the other identical site
(the second helper around the block referencing
setActiveMainWindow/windowForMainWindowId/TerminalController.shared) so both
sync helpers only return early when the explorer state matches too.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 5085-5096: When reopening we must deminiaturize a reused window
because makeKeyAndOrderFront(nil) doesn't always restore a miniaturized window;
update ensureInitialMainWindowIfNeeded to call window.deminiaturize(nil) on the
resolvedWindow (before makeKeyAndOrderFront(nil)), then proceed to
makeKeyAndOrderFront(nil) and setActiveMainWindow(window); keep the existing
fallback to createMainWindow(shouldActivate:) when no context/window is found.
- Around line 4680-4708: When removing an active MainWindowContext you must also
repoint or clear AppDelegate.fileExplorerState like you do for tabManager,
sidebarState and sidebarSelectionState to avoid it referencing a deallocated
window; in discardOrphanedMainWindowContext(_:) (and similarly in
unregisterMainWindow(_:)) after selecting nextContext (the one found via
mainWindowContexts.values.first(where: { resolvedWindow(for: $0) != nil })) set
fileExplorerState = nextContext.fileExplorerState (and when there is no
nextContext set fileExplorerState = nil), ensuring this update happens alongside
the existing tabManager/sidebar updates and
TerminalController.shared.setActiveTabManager calls so an early-return later
won’t leave fileExplorerState pointing at the dead window.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
3316-3328:⚠️ Potential issue | 🟠 MajorTreat minimized originals as canonical in the duplicate-registration guard.
Line 3318 still gates the fast-return path on
existingWindow.isVisibleonly. If the original main window is minimized, this branch can still fall through, rebind thewindowIdto the new window, and leave the old one alive — which reopens the duplicate-window path this PR is trying to close.Suggested fix
- if let existingWindow = existing.window, - existingWindow !== window, - existingWindow.isVisible { + if let existingWindow = existing.window, + existingWindow !== window, + (existingWindow.isVisible || existingWindow.isMiniaturized) { `#if` DEBUG cmuxDebugLog( "mainWindow.register.duplicateIgnored windowId=\(String(windowId.uuidString.prefix(8))) " + "existing={\(debugWindowToken(existingWindow))} duplicate={\(debugWindowToken(window))}" )In AppKit, does `NSWindow.isVisible` exclude miniaturized windows, requiring a separate `isMiniaturized` check when deciding whether an already-registered window is still the live canonical window?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 3316 - 3328, The duplicate-registration guard currently fast-returns only when existingWindow.isVisible is true, which misses the case where the canonical window is miniaturized; update the condition to treat a miniaturized existing window as canonical (e.g., check existingWindow.isVisible || existingWindow.isMiniaturized) so you keep existing.tabManager.window bound to existingWindow and close the new window; update any debug message around mainWindow.register.duplicateIgnored (uses windowId and debugWindowToken(window)) as needed to reflect the tightened condition.
🤖 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/cmuxApp.swift`:
- Around line 1020-1026: The bootstrap hidden window is being given an
identifier that falls under the "cmux.main.*" namespace causing accidental
matches; change the identifier set in the WindowAccessor closure (where
window.identifier = NSUserInterfaceItemIdentifier("cmux.main.bootstrap")) to a
name outside that namespace (for example "cmux.bootstrap" or
"cmux.hidden.bootstrap") so it no longer starts with "cmux.main"; also search
for any other uses of "cmux.main.bootstrap" and update them to the new
identifier so TabManager.closeConfirmationPresentingWindow() and any other logic
that only intends to match "cmux.main" / "cmux.main.*" won’t pick up the
bootstrap window.
- Around line 189-197: The startup currently calls bootstrapMainWindowScene()
from MainWindowBootstrapView.onAppear but omits the prior per-window state/setup
(config, file-explorer/directory) that used to run on WindowGroup startup;
update the bootstrap so that AppDelegate.bootstrapInitialMainWindowIfNeeded(...)
or the AppDelegate.registerMainWindow path performs the full per-window
initialization (including creating and wiring FileBrowserDrawerState and
file/explorer/directory config) before TerminalController starts, or
alternatively invoke a dedicated bootstrap step here (from
MainWindowBootstrapView.onAppear) that performs the old initialization then
hands the fully-initialized FileBrowserDrawerState into TerminalController;
ensure consistency with AppDelegate.createMainWindow and ContentView.onAppear
ownership patterns so primary windows register via
AppDelegate.registerMainWindow with SwiftUI-owned FileBrowserDrawerState and
secondary windows created via AppDelegate.createMainWindow construct their own
FileBrowserDrawerState.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 3316-3328: The duplicate-registration guard currently fast-returns
only when existingWindow.isVisible is true, which misses the case where the
canonical window is miniaturized; update the condition to treat a miniaturized
existing window as canonical (e.g., check existingWindow.isVisible ||
existingWindow.isMiniaturized) so you keep existing.tabManager.window bound to
existingWindow and close the new window; update any debug message around
mainWindow.register.duplicateIgnored (uses windowId and
debugWindowToken(window)) as needed to reflect the tightened condition.
🪄 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: 2ced69bf-c5ba-4eb6-9c41-b0efc0d0cd95
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/cmuxApp.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 557b105b45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func applicationShouldHandleReopen(_ sender: NSApplication, hasVisibleWindows flag: Bool) -> Bool { | ||
| ensureInitialMainWindowIfNeeded() | ||
| return true |
There was a problem hiding this comment.
applicationShouldHandleReopen ignores hasVisibleWindows and always calls ensureInitialMainWindowIfNeeded(), which promotes the first registered main-window context to key. With multiple open main windows, reopening from the Dock while the app is inactive can therefore switch the active TabManager to an arbitrary window (UUID-order fallback) and misroute subsequent menu/socket actions to the wrong window. This handler should only force-create/activate when no main window is visible, or preserve the current key/main window when one already exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 39dac431. Reopen now only creates or activates a main terminal window when none is visible; if a main terminal window is already visible, it preserves the current key/main routing instead of selecting a fallback context.
— Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39dac431fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SystemWideHotkeyController.shared.start() | ||
| NSApp.servicesProvider = self | ||
|
|
||
| bootstrapInitialMainWindowIfNeeded(debugSource: "didFinishLaunching") |
There was a problem hiding this comment.
Calling bootstrapInitialMainWindowIfNeeded during applicationDidFinishLaunching can trigger createMainWindow/registerMainWindow before launch-time URL opens are handled, which means attemptStartupSessionRestoreIfNeeded can run while didHandleExplicitOpenIntentAtStartup is still false. In the common LaunchServices order where application(_:open:) arrives after launch (for example open -a ... <folder>), explicit folder opens no longer suppress startup session restore, so users can get restored windows/workspaces in addition to the requested directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 57220bd. Initial main-window bootstrap is now coalesced and scheduled from both AppDelegate launch and the SwiftUI placeholder scene, so launch-time folder-open delivery can mark the explicit open before startup session restore runs.
— Claude Code
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This branch was successfully deployed
1 participant
Fixes the duplicate-window path behind external folder opens.
What changed:
Verification:
wmain./usr/bin/open -a <wmain.app> <folder>for/var/tmp,~,~/fun,~/fun/cmuxterm-hq, and/tmp.cmux DEV wmainwindow./tmpworkspace showed a rendered terminal prompt.Summary by CodeRabbit
Bug Fixes
Improvements