Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions Resources/Localizable.xcstrings
Original file line number Diff line number Diff line change
Expand Up @@ -77797,6 +77797,40 @@
}
}
}
},
"update.relaunchWarning.message": {
"extractionState": "manual",
"localizations": {
"en": {
"stringUnit": {
"state": "translated",
"value": "This update will relaunch cmux and terminate in-app terminal sessions.\n\nWorkspaces affected: %1$lld\nTerminal sessions affected: %2$lld\nRunning commands detected: %3$lld\nRemote / SSH terminal sessions: %4$lld\n\nShells, SSH connections, and agents attached to these PTYs will be terminated."
}
},
"ja": {
"stringUnit": {
"state": "translated",
"value": "このアップデートを適用すると cmux が再起動し、アプリ内のターミナルセッションが終了します。\n\n影響を受けるワークスペース: %1$lld\n影響を受けるターミナルセッション: %2$lld\n実行中コマンドの検出数: %3$lld\nリモート / SSH ターミナルセッション: %4$lld\n\nこれらの PTY に接続されたシェル、SSH 接続、エージェントは終了します。"
}
}
}
},
"update.relaunchWarning.title": {
"extractionState": "manual",
"localizations": {
"en": {
"stringUnit": {
"state": "translated",
"value": "Update Will Close Terminal Sessions"
}
},
"ja": {
"stringUnit": {
"state": "translated",
"value": "アップデートでターミナルセッションが終了します"
}
}
}
}
}
}
115 changes: 115 additions & 0 deletions Sources/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1994,6 +1994,13 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
let display: SessionDisplaySnapshot?
}

private struct UpdateRelaunchTerminalSummary {
let workspaceCount: Int
let terminalSessionCount: Int
let runningCommandCount: Int
let remoteTerminalSessionCount: Int
}

private static let persistedWindowGeometryDefaultsKey = "cmux.session.lastWindowGeometry.v1"

weak var tabManager: TabManager?
Expand Down Expand Up @@ -5889,6 +5896,59 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
updateController.attemptUpdate()
}

func confirmUpdateRelaunchIfNeeded() -> Bool {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing @MainActor annotation

confirmUpdateRelaunchIfNeeded() calls NSAlert.runModal() (which must run on the main thread) and reads main-thread-only state via updateRelaunchTerminalSummary() (mainWindowContexts, tabManager). The current call site wraps it in Task { @MainActor } correctly, but without the annotation the compiler won't prevent future callers from invoking this method off-actor.

Suggested change
func confirmUpdateRelaunchIfNeeded() -> Bool {
@MainActor
func confirmUpdateRelaunchIfNeeded() -> Bool {

guard let summary = updateRelaunchTerminalSummary() else {
UpdateLogStore.shared.append("update relaunch warning skipped (no active terminal sessions)")
return true
}

UpdateLogStore.shared.append(
"update relaunch warning shown " +
"(workspaces=\(summary.workspaceCount), sessions=\(summary.terminalSessionCount), " +
"running=\(summary.runningCommandCount), remote=\(summary.remoteTerminalSessionCount))"
)

let alert = NSAlert()
alert.alertStyle = .warning
alert.messageText = String(
localized: "update.relaunchWarning.title",
defaultValue: "Update Will Close Terminal Sessions"
)
alert.informativeText = String(
localized: "update.relaunchWarning.message",
defaultValue: """
This update will relaunch cmux and terminate in-app terminal sessions.

Workspaces affected: \(summary.workspaceCount)
Terminal sessions affected: \(summary.terminalSessionCount)
Running commands detected: \(summary.runningCommandCount)
Remote / SSH terminal sessions: \(summary.remoteTerminalSessionCount)

Shells, SSH connections, and agents attached to these PTYs will be terminated.
"""
Comment on lines +5917 to +5928
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use plural-aware localized strings for the count lines.

This packs four counted labels into one fixed-plural paragraph, so translators cannot independently handle 1 vs n for each line. Split these into separate .one / .other resources (or otherwise plural-aware localized lines) before composing the alert body.

Based on learnings: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/AppDelegate.swift` around lines 5912 - 5923, The
alert.informativeText is using a single localized paragraph
("update.relaunchWarning.message") with four embedded counts
(summary.workspaceCount, summary.terminalSessionCount,
summary.runningCommandCount, summary.remoteTerminalSessionCount) which prevents
proper plural handling; change to use separate plural-aware localization keys
(e.g. "update.relaunchWarning.workspaces", ".terminalSessions",
".runningCommands", ".remoteTerminalSessions") each with .one/.other forms, then
build alert.informativeText by composing these four localized plural strings
plus the static header lines so each count can be pluralized independently;
update the call-site that constructs alert.informativeText to call
String(localized: "update.relaunchWarning.workspaces", ... ,
summary.workspaceCount) etc., and remove the single combined
"update.relaunchWarning.message" usage.

)
alert.addButton(withTitle: String(localized: "common.restartNow", defaultValue: "Restart Now"))
alert.addButton(withTitle: String(localized: "common.later", defaultValue: "Later"))

if let updateNowButton = alert.buttons.first {
updateNowButton.keyEquivalent = "\r"
updateNowButton.keyEquivalentModifierMask = []
alert.window.defaultButtonCell = updateNowButton.cell as? NSButtonCell
alert.window.initialFirstResponder = updateNowButton
}
if let deferButton = alert.buttons.dropFirst().first {
deferButton.keyEquivalent = "\u{1b}"
}

if NSApp.activationPolicy() == .regular {
NSApp.activate(ignoringOtherApps: true)
}

let confirmed = alert.runModal() == .alertFirstButtonReturn
UpdateLogStore.shared.append("update relaunch warning result=\(confirmed ? "confirm" : "defer")")
return confirmed
}

func isCmuxCLIInstalledInPATH() -> Bool {
CmuxCLIPathInstaller().isInstalled()
}
Expand Down Expand Up @@ -5958,6 +6018,61 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
}
}

private func updateRelaunchTerminalSummary() -> UpdateRelaunchTerminalSummary? {
var seenManagers = Set<ObjectIdentifier>()
var managers: [TabManager] = []

for context in mainWindowContexts.values {
let managerId = ObjectIdentifier(context.tabManager)
guard seenManagers.insert(managerId).inserted else { continue }
managers.append(context.tabManager)
}

if managers.isEmpty, let tabManager {
managers.append(tabManager)
}
Comment on lines +6021 to +6033
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Do not hide the active tabManager behind an empty-array fallback.

This only appends tabManager when mainWindowContexts is empty. If window/context bookkeeping is temporarily out of sync, the active manager's terminals can be omitted from the summary and the relaunch warning can undercount or disappear. The existing forEachTerminalPanel helper later in this file already uses the safer “visit both sources, then dedupe by identity” pattern.

♻️ Suggested adjustment
         for context in mainWindowContexts.values {
             let managerId = ObjectIdentifier(context.tabManager)
             guard seenManagers.insert(managerId).inserted else { continue }
             managers.append(context.tabManager)
         }

-        if managers.isEmpty, let tabManager {
-            managers.append(tabManager)
+        if let tabManager {
+            let managerId = ObjectIdentifier(tabManager)
+            if seenManagers.insert(managerId).inserted {
+                managers.append(tabManager)
+            }
         }
📝 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.

Suggested change
private func updateRelaunchTerminalSummary() -> UpdateRelaunchTerminalSummary? {
var seenManagers = Set<ObjectIdentifier>()
var managers: [TabManager] = []
for context in mainWindowContexts.values {
let managerId = ObjectIdentifier(context.tabManager)
guard seenManagers.insert(managerId).inserted else { continue }
managers.append(context.tabManager)
}
if managers.isEmpty, let tabManager {
managers.append(tabManager)
}
private func updateRelaunchTerminalSummary() -> UpdateRelaunchTerminalSummary? {
var seenManagers = Set<ObjectIdentifier>()
var managers: [TabManager] = []
for context in mainWindowContexts.values {
let managerId = ObjectIdentifier(context.tabManager)
guard seenManagers.insert(managerId).inserted else { continue }
managers.append(context.tabManager)
}
if let tabManager {
let managerId = ObjectIdentifier(tabManager)
if seenManagers.insert(managerId).inserted {
managers.append(tabManager)
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/AppDelegate.swift` around lines 6016 - 6028, The function
updateRelaunchTerminalSummary currently only appends the active tabManager when
managers.isEmpty, which can omit the active manager if mainWindowContexts is
temporarily out of sync; change the collection logic in
updateRelaunchTerminalSummary to always include both sources (iterate
mainWindowContexts and also include the optional tabManager) and then dedupe by
identity (use ObjectIdentifier like the existing seenManagers set) so every
unique TabManager is visited exactly once; reference the symbols
mainWindowContexts, tabManager, seenManagers, and the
updateRelaunchTerminalSummary function and mirror the “visit both sources, then
dedupe by identity” pattern used by forEachTerminalPanel.


var workspaceCount = 0
var terminalSessionCount = 0
var runningCommandCount = 0
var remoteTerminalSessionCount = 0

for manager in managers {
for workspace in manager.tabs {
var workspaceHasTerminalSession = false

for (panelId, panel) in workspace.panels {
guard let terminalPanel = panel as? TerminalPanel else { continue }
workspaceHasTerminalSession = true
terminalSessionCount += 1

if workspace.panelNeedsConfirmClose(
panelId: panelId,
fallbackNeedsConfirmClose: terminalPanel.needsConfirmClose()
) {
runningCommandCount += 1
Comment on lines +6049 to +6053
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

This is a busy-session count, not a command count.

This increments once per terminal panel based on panelNeedsConfirmClose(...); Sources/Workspace.swift:6074-6079 backs that with panelShellActivityStates, so the downstream “Running commands detected” wording reads more precise than the data you actually have. Rename the metric/message to something like “sessions requiring close confirmation,” or switch to a real command enumeration source.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/AppDelegate.swift` around lines 6044 - 6048, The code is incrementing
runningCommandCount for panels where
workspace.panelNeedsConfirmClose(panelId:fallbackNeedsConfirmClose:) (which uses
panelShellActivityStates in Workspace.swift) is true — this represents sessions
requiring close confirmation, not actual running commands; change the
metric/variable and user-facing text accordingly (e.g., rename
runningCommandCount to sessionsRequiringCloseConfirmationCount and update any
log/message like "Running commands detected" to "Sessions requiring close
confirmation") or, if you need true command counts, replace the increment with
logic that queries the real command enumeration source instead of
terminalPanel.needsConfirmClose() / panelShellActivityStates.

}

Comment on lines +6048 to +6055
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 runningCommandCount counts panels, not commands

The counter increments once per TerminalPanel whose panelNeedsConfirmClose returns true, but the UI label reads "Running commands detected: N" — implying N individual commands. If a single terminal panel hosts a pipeline of several processes, the number displayed will be lower than the actual running-command count, which could cause users to underestimate the impact of the restart.

Consider renaming the field and label to "Terminal sessions with running commands:" to match what the code actually measures, or documenting the known limitation in a comment.

if workspace.isRemoteTerminalSurface(panelId) {
remoteTerminalSessionCount += 1
}
}

if workspaceHasTerminalSession {
workspaceCount += 1
}
}
}

guard terminalSessionCount > 0 else { return nil }
return UpdateRelaunchTerminalSummary(
workspaceCount: workspaceCount,
terminalSessionCount: terminalSessionCount,
runningCommandCount: runningCommandCount,
remoteTerminalSessionCount: remoteTerminalSessionCount
)
}

@objc func restartSocketListener(_ sender: Any?) {
guard tabManager != nil else {
NSSound.beep()
Expand Down
6 changes: 5 additions & 1 deletion Sources/Update/UpdateDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,15 @@ extension UpdateDriver: SPUUpdaterDelegate {
/// Called when an update is scheduled to install silently,
/// which occurs when automatic download is enabled.
func updater(_ updater: SPUUpdater, willInstallUpdateOnQuit item: SUAppcastItem, immediateInstallationBlock immediateInstallHandler: @escaping () -> Void) -> Bool {
let guardedImmediateInstallHandler = makeGuardedUpdateRelaunchAction(
applicationTerminated: false,
action: immediateInstallHandler
)
DispatchQueue.main.async { [weak viewModel] in
viewModel?.clearDetectedUpdate()
viewModel?.state = .installing(.init(
isAutoUpdate: true,
retryTerminatingApplication: immediateInstallHandler,
retryTerminatingApplication: guardedImmediateInstallHandler,
dismiss: { [weak viewModel] in
viewModel?.state = .idle
}
Expand Down
98 changes: 87 additions & 11 deletions Sources/Update/UpdateDriver.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class UpdateDriver: NSObject, SPUUserDriver {
private var lastFeedURLString: String?
private var pendingUserInitiatedCheckPresentation: UpdateUserInitiatedCheckPresentation?
private var activeUserInitiatedCheckPresentation: UpdateUserInitiatedCheckPresentation?
var confirmUpdateRelaunchIfNeededHandler: (() -> Bool)?

init(viewModel: UpdateViewModel, hostBundle: Bundle) {
self.viewModel = viewModel
Expand Down Expand Up @@ -71,17 +72,21 @@ class UpdateDriver: NSObject, SPUUserDriver {
state: SPUUserUpdateState,
reply: @escaping @Sendable (SPUUserUpdateChoice) -> Void) {
UpdateLogStore.shared.append("show update found: \(appcastItem.displayVersionString)")
let forwardedReply: @Sendable (SPUUserUpdateChoice) -> Void = { [weak self] choice in
if choice != .install {
self?.finishUserInitiatedCheckPresentation()
}
reply(choice)
}
let guardedReply = shouldGuardInstallChoice(for: state)
? makeGuardedUpdateInstallChoiceReply(forwardedReply)
: forwardedReply
if usesStandardPresentation {
clearCustomStateForStandardPresentation()
standard.showUpdateFound(with: appcastItem, state: state) { [weak self] choice in
if choice != .install {
self?.finishUserInitiatedCheckPresentation()
}
reply(choice)
}
standard.showUpdateFound(with: appcastItem, state: state, reply: guardedReply)
return
}
setStateAfterMinimumCheckDelay(.updateAvailable(.init(appcastItem: appcastItem, reply: reply)))
setStateAfterMinimumCheckDelay(.updateAvailable(.init(appcastItem: appcastItem, reply: guardedReply)))
}

func showUpdateReleaseNotes(with downloadData: SPUDownloadData) {
Expand Down Expand Up @@ -208,21 +213,35 @@ class UpdateDriver: NSObject, SPUUserDriver {

func showReady(toInstallAndRelaunch reply: @escaping @Sendable (SPUUserUpdateChoice) -> Void) {
UpdateLogStore.shared.append("show ready to install")
let forwardedReply: @Sendable (SPUUserUpdateChoice) -> Void = { [weak self] choice in
if choice != .install {
self?.finishUserInitiatedCheckPresentation()
}
reply(choice)
}
let guardedReply = makeGuardedUpdateInstallChoiceReply(forwardedReply)
if usesStandardPresentation {
standard.showReady(toInstallAndRelaunch: reply)
standard.showReady(toInstallAndRelaunch: guardedReply)
return
}
reply(.install)
guardedReply(.install)
}

func showInstallingUpdate(withApplicationTerminated applicationTerminated: Bool, retryTerminatingApplication: @escaping () -> Void) {
UpdateLogStore.shared.append("show installing update")
let guardedRetryTerminatingApplication = makeGuardedUpdateRelaunchAction(
applicationTerminated: applicationTerminated,
action: retryTerminatingApplication
)
if usesStandardPresentation {
standard.showInstallingUpdate(withApplicationTerminated: applicationTerminated, retryTerminatingApplication: retryTerminatingApplication)
standard.showInstallingUpdate(
withApplicationTerminated: applicationTerminated,
retryTerminatingApplication: guardedRetryTerminatingApplication
)
return
}
setState(.installing(.init(
retryTerminatingApplication: retryTerminatingApplication,
retryTerminatingApplication: guardedRetryTerminatingApplication,
dismiss: { [weak viewModel] in
viewModel?.state = .idle
}
Expand Down Expand Up @@ -457,6 +476,63 @@ class UpdateDriver: NSObject, SPUUserDriver {
}
}

private func shouldGuardInstallChoice(for state: SPUUserUpdateState) -> Bool {
state.stage != .notDownloaded
}

@MainActor
private func confirmUpdateRelaunchIfNeeded() -> Bool {
confirmUpdateRelaunchIfNeededHandler?() ?? AppDelegate.shared?.confirmUpdateRelaunchIfNeeded() ?? true
}

func makeGuardedUpdateInstallChoiceReply(
_ reply: @escaping @Sendable (SPUUserUpdateChoice) -> Void
) -> @Sendable (SPUUserUpdateChoice) -> Void {
{ [weak self] choice in
guard choice == .install else {
reply(choice)
return
}

Task { @MainActor [weak self] in
let confirmed = self?.confirmUpdateRelaunchIfNeeded() ?? true
guard confirmed else {
self?.recordDeferredUpdateRelaunch()
reply(.dismiss)
return
}

reply(.install)
}
}
}

func makeGuardedUpdateRelaunchAction(
applicationTerminated: Bool,
action: @escaping () -> Void
) -> () -> Void {
{ [weak self] in
Task { @MainActor [weak self] in
if applicationTerminated {
action()
return
}

let confirmed = self?.confirmUpdateRelaunchIfNeeded() ?? true
guard confirmed else {
self?.recordDeferredUpdateRelaunch()
return
}

action()
}
}
Comment on lines +514 to +529
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Redundant outer [weak self] capture

The outer closure captures [weak self] but never references self directly — only the inner Task closure uses it (and re-captures [weak self] independently). The outer capture has no effect and creates a misleading impression that self is used in the outer scope.

Suggested change
{ [weak self] in
Task { @MainActor [weak self] in
guard self != nil else { return }
if applicationTerminated {
action()
return
}
guard AppDelegate.shared?.confirmUpdateRelaunchIfNeeded() ?? true else {
UpdateLogStore.shared.append("update relaunch deferred due to active terminal sessions")
return
}
action()
}
}
{ in
Task { @MainActor [weak self] in
guard self != nil else { return }
if applicationTerminated {
action()
return
}
guard AppDelegate.shared?.confirmUpdateRelaunchIfNeeded() ?? true else {
UpdateLogStore.shared.append("update relaunch deferred due to active terminal sessions")
return
}
action()
}
}

}

private func recordDeferredUpdateRelaunch() {
UpdateLogStore.shared.append("update relaunch deferred due to active terminal sessions")
}

private func runOnMain(_ action: @escaping () -> Void) {
if Thread.isMainThread {
action()
Expand Down
Loading