Add zmx-backed workspace/session persistence#510
Add zmx-backed workspace/session persistence#510mdsakalu wants to merge 10 commits intomanaflow-ai:mainfrom
Conversation
|
@mdsakalu is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
mdsakalu
force-pushed
the
push-xvtoqysospul
branch
2 times, most recently
from
6bb551a to
bc28054
Compare
|
@codex review |
![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: bc28054337
ℹ️ 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".
Sources/Workspace.swift
Outdated
| let panel = BrowserPanel(workspaceId: id, initialURL: url) | ||
| panels[panel.id] = panel | ||
| panelTitles[panel.id] = panel.displayTitle |
There was a problem hiding this comment.
The restore path creates BrowserPanel instances in rebuildNode but does not call installBrowserPanelSubscription, unlike normal browser creation/attach flows (newBrowserSurface, newBrowserSplit, attachDetachedSurface). This means restored browser tabs won't propagate ongoing title/loading/favicon changes back to Bonsplit, so tab UI can stay stale after relaunch until tabs are recreated.
Useful? React with 👍 / 👎.
| let workspaceSnapshots = tabManager.tabs.map { workspace in | ||
| workspace.generateSnapshot() | ||
| } |
There was a problem hiding this comment.
WorkspaceStatePersistence.save(tabManager:) writes a single global workspace-state.json from only tabManager.tabs. Since the app maintains multiple main-window contexts (each with its own TabManager), any save triggered from one manager overwrites the file and drops workspaces from other open windows, causing cross-window state loss on next launch.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc28054337
ℹ️ 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".
Sources/Workspace.swift
Outdated
| let panel = BrowserPanel(workspaceId: id, initialURL: url) | ||
| panels[panel.id] = panel | ||
| panelTitles[panel.id] = panel.displayTitle | ||
|
|
There was a problem hiding this comment.
The restore path rebuilds BrowserPanel instances but never calls installBrowserPanelSubscription, unlike the normal browser-creation paths (newBrowserSurface/newBrowserSplit). In sessions restored from snapshot, browser tabs therefore lose title/loading/favicon propagation to Bonsplit after relaunch, so tab metadata can remain stale even though the web view state changes.
Useful? React with 👍 / 👎.
| guard let pathEnv = ProcessInfo.processInfo.environment["PATH"] else { return false } | ||
| let dirs = pathEnv.split(separator: ":") |
There was a problem hiding this comment.
isZmxAvailable() only scans ProcessInfo.processInfo.environment["PATH"], which can be minimal in GUI-launched macOS apps (e.g., missing Homebrew directories). In that environment this returns false despite zmx being installed, and startup then restores with zmxEnabled = false and suppresses subsequent saves, effectively turning off the new persistence flow for affected users.
Useful? React with 👍 / 👎.
mdsakalu
force-pushed
the
push-xvtoqysospul
branch
from
bc28054 to
9c5e6e5
Compare
|
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 zmx session persistence: new ZmxSupport utilities, Settings toggles and localizations, Ghostty C API zmx/io extensions, deterministic per-panel zmx naming and snapshot persistence, wiring names into terminal surfaces, initial-present retry/attachment wiring, and workspace-close zmx cleanup. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Workspace
participant ZmxNaming as ZmxSessionNaming
participant TerminalPanel
participant TerminalSurface
participant GhosttyC as Ghostty C API
User->>Workspace: request newTerminalSurface(zmxSessionName?)
alt no zmxSessionName provided
Workspace->>ZmxNaming: sessionName(stableId, panelIndex)
ZmxNaming-->>Workspace: "cmux-{stableId}-{index}"
end
Workspace->>TerminalPanel: init(zmxSessionName, zmxCreate)
TerminalPanel->>TerminalSurface: configure(zmx_session, zmx_create)
TerminalSurface->>GhosttyC: create surface with config
GhosttyC-->>TerminalSurface: surface_t
Workspace->>Workspace: store zmxSessionNames[panelId]
sequenceDiagram
participant TabManager
participant Workspace
participant ZmxProbe as ZmxSessionProbe
participant UnixSocket as Unix Socket
TabManager->>Workspace: closeWorkspace()
Workspace->>Workspace: killZmxSessions()
loop for each zmxSessionName
Workspace->>ZmxProbe: killSession(sessionName)
ZmxProbe->>UnixSocket: resolve socket path + non-blocking connect
ZmxProbe->>UnixSocket: write kill IPC (header + tag)
UnixSocket-->>ZmxProbe: close
end
Workspace->>Workspace: teardown panels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
Sources/GhosttyTerminalView.swift (1)
2026-2029: Prefer constructor-only zmx surface settings.These values are only consumed during the initial
createSurface(for:)path, so leaving them mutable makes persistence depend on post-init mutation and easy to miss on futureTerminalSurface(...)call sites. Passing them throughinit(...)and making them immutable would enforce the zmx contract at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 2026 - 2029, zmxSessionName and zmxCreate are mutable properties but only used during initial surface creation; make them constructor-only to enforce immutability. Change the TerminalSurface (or GhosttyTerminalView) initializer to accept zmxSessionName: String? and zmxCreate: Bool (defaulting to true if desired), store them in let constants (rename to zmxSessionName and zmxCreate if needed) instead of var, and remove external mutations; ensure createSurface(for:) reads these immutable properties. Update any call sites that construct TerminalSurface(...) to pass the zmx values through the new init parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitmodules:
- Around line 3-4: Update the submodule and docs to meet repo policy: push the
local submodule commit 9c5e6e58f3fc3c298be1dc8e7fbdaa1beaf0746f to the remote
https://github.com/mdsakalu/ghostty.git on branch zmx-termio-backend so the
parent repo can safely reference it (ensure git push origin zmx-termio-backend
contains that commit), and rewrite docs/ghostty-fork.md to replace references to
manaflow-ai/ghostty with mdsakalu/ghostty, clearly describe the fork’s purpose
(zmx-termio-backend support), list the specific changes (e.g., OSC 99
parser/display link restart behavior if applicable or note they differ), and add
any merge-conflict notes relevant to that branch before re-submitting the PR.
In `@Sources/cmuxApp.swift`:
- Around line 3721-3746: Replace the bare string literals in this SwiftUI block
with localized strings using String(localized:defaultValue:); specifically
update the title passed to SettingsSectionHeader ("zmx Session Persistence"),
the labels and subtitles in the two SettingsToggleRow calls ("Enable zmx Session
Persistence", both subtitle variants, "Kill Sessions on Workspace Close", both
subtitle variants), and the SettingsCardNote text ("Requires zmx to be installed
and on PATH."). Use consistent localization keys (e.g.,
"settings.zmx.persistence.title", "settings.zmx.persistence.enable.title",
"settings.zmx.persistence.enable.subtitle.enabled"/".disabled",
"settings.zmx.persistence.killOnClose.title",
"settings.zmx.persistence.killOnClose.subtitle.enabled"/".disabled",
"settings.zmx.persistence.note.requirements") and wrap each literal with
String(localized: "key", defaultValue: "English text"); keep the conditional
logic with ZmxSessionProbe.isZmxAvailable() unchanged.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2478-2485: The current code only overwrites
surfaceConfig.zmx_session and surfaceConfig.zmx_create when zmxSessionName is
non-nil, so cloned/split panels inheriting configTemplate may keep the source
zmx values; explicitly clear those fields when zmxSessionName is nil by adding
an else branch that sets surfaceConfig.zmx_session = nil (NULL) and
surfaceConfig.zmx_create = 0 (or false depending on type) so the new
surfaceConfig does not retain the previous session; make this change near the
zmxSessionName handling in GhosttyTerminalView.swift (affecting
surfaceConfig.zmx_session, surfaceConfig.zmx_create and the existing strdup/free
logic).
In `@Sources/TabManager.swift`:
- Line 1227: The final workspace close path bypasses closeWorkspace(_:) so
ZmxPersistenceSettings.killOnWorkspaceClose isn't honored; update
unregisterMainWindow() in AppDelegate to perform the same zmx teardown as
closeWorkspace(_:): call workspace.killZmxSessions() (or the equivalent
teardown) before removing the window context/notifications, ensuring the same
behavior as when closeWorkspace(_:) is used and covering
AppDelegate.closeMainWindowContainingTabId(...) flows.
In `@Sources/Workspace.swift`:
- Around line 2112-2114: The zmxSessionNames map entry for a panel is being set
in the createTab flow (zmxSessionNames[newPanel.id] = resolvedZmxName) but never
cleared or transferred when panels are moved/closed; update
splitTabBar(_:didCloseTab:), splitTabBar(_:didClosePane:), and
attachDetachedSurface(...) to either remove the zmxSessionNames entry from the
source or transfer it to the destination panel id when ownership moves, and
ensure createTab failure rolls back the map entry (remove
zmxSessionNames[newPanel.id]) so stale ownership is not left behind; make sure
killZmxSessions() semantics then operate only on entries that belong to this
workspace.
- Around line 508-518: The Workspace restore currently calls
ZmxSessionProbe.isZmxAvailable() and isSessionAlive(name) on the `@MainActor`,
which can block the UI; change the logic in the restore path so these probes run
off-main (e.g., dispatch to a background Task or use Task.detached) and return
their results asynchronously to the surface instead of polling on the main
actor. Specifically, stop calling ZmxSessionProbe.isSessionAlive(name) and
ZmxSessionProbe.isZmxAvailable() directly when computing zmxName/zmxCreate
inside Workspace; instead compute zmxName synchronously from
snapshot.zmxSessionName and ZmxPersistenceSettings.isEnabled, set a conservative
default for zmxCreate (e.g., true), and kick off an async background probe that
updates the surface/state when the detached Task determines availability and
liveness using ZmxSessionProbe APIs. Ensure identifiers referenced are
Workspace, snapshot.zmxSessionName, ZmxPersistenceSettings.isEnabled,
ZmxSessionProbe.isZmxAvailable(), ZmxSessionProbe.isSessionAlive(name), zmxName,
and zmxCreate.
- Around line 2083-2093: Several code paths construct TerminalPanel directly and
bypass the zmx parameter resolution (newTerminalSurface handles it), causing
those terminals to be non-reattachable; create a single helper (or extend
newTerminalSurface) that encapsulates the zmx resolution logic (use
ZmxPersistenceSettings.isEnabled, ZmxSessionProbe.isZmxAvailable(),
zmxSessionName, zmxStableId, zmxNextPanelIndex and ZmxSessionNaming.sessionName
to produce resolvedZmxName and update zmxStableId/zmxNextPanelIndex as shown)
and replace all direct TerminalPanel construction sites (references at/around
the places that currently call TerminalPanel directly such as the flows at lines
you noted) to call this helper so every TerminalPanel is constructed with the
same resolvedZmxName and associated zmx params.
In `@Sources/ZmxSupport.swift`:
- Around line 156-158: After creating the socket (the fd used for connect() in
ZmxSupport.swift) set SO_NOSIGPIPE on the socket immediately (following the
pattern in TerminalController.swift) via setsockopt to prevent Darwin from
raising SIGPIPE on a write to a closed Unix stream; then replace the unchecked
send(fd, &msg, msg.count, 0) call with a checked send and handle its return
value to detect short writes or errors (treat return < msg.count or < 0 as a
failure and log/return an error accordingly). Ensure this is done right after
socket creation and before the connect/send sequence so SIGPIPE cannot terminate
the process.
---
Nitpick comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2026-2029: zmxSessionName and zmxCreate are mutable properties but
only used during initial surface creation; make them constructor-only to enforce
immutability. Change the TerminalSurface (or GhosttyTerminalView) initializer to
accept zmxSessionName: String? and zmxCreate: Bool (defaulting to true if
desired), store them in let constants (rename to zmxSessionName and zmxCreate if
needed) instead of var, and remove external mutations; ensure
createSurface(for:) reads these immutable properties. Update any call sites that
construct TerminalSurface(...) to pass the zmx values through the new init
parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98b1cee9-1235-40ec-a193-25f2da63135e
📒 Files selected for processing (10)
.gitmodulesGhosttyTabs.xcodeproj/project.pbxprojSources/GhosttyTerminalView.swiftSources/Panels/TerminalPanel.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/Workspace.swiftSources/ZmxSupport.swiftSources/cmuxApp.swiftghostty.h
| url = https://github.com/mdsakalu/ghostty.git | ||
| branch = zmx-termio-backend |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the ghostty submodule commit exists on the remote fork
# Get the submodule commit SHA
cd ghostty 2>/dev/null && COMMIT=$(git rev-parse HEAD) && cd ..
if [ -n "$COMMIT" ]; then
echo "Submodule commit: $COMMIT"
# Check if commit exists on remote
git ls-remote https://github.com/mdsakalu/ghostty.git | grep -q "$COMMIT" && echo "✓ Commit exists on remote" || echo "✗ Commit NOT found on remote"
else
echo "Could not determine submodule commit"
fi
# Check if fork documentation exists
echo ""
echo "Checking for fork documentation:"
fd -t f "ghostty-fork.md" docs/ 2>/dev/null || echo "docs/ghostty-fork.md not found"Repository: manaflow-ai/cmux
Length of output: 202
🏁 Script executed:
cat docs/ghostty-fork.mdRepository: manaflow-ai/cmux
Length of output: 1724
This PR violates critical submodule management requirements and cannot be merged as-is.
Two blocking issues:
-
Submodule commit not on remote: The local commit
9c5e6e58f3fc3c298be1dc8e7fbdaa1beaf0746fmust be pushed tohttps://github.com/mdsakalu/ghostty.giton thezmx-termio-backendbranch before updating the parent repo pointer. -
Fork documentation out of date:
docs/ghostty-fork.mdstill documentsmanaflow-ai/ghosttyand its changes (OSC 99 parser, display link restart). This file must be rewritten to document themdsakalu/ghosttyfork, its purpose (zmx-termio-backend support), the changes it contains, and any merge conflict notes relevant to that branch.
Per repository guidelines: submodule commits must exist on their remote branch before the parent repo can reference them, and fork documentation must be kept in sync with actual fork changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitmodules around lines 3 - 4, Update the submodule and docs to meet repo
policy: push the local submodule commit 9c5e6e58f3fc3c298be1dc8e7fbdaa1beaf0746f
to the remote https://github.com/mdsakalu/ghostty.git on branch
zmx-termio-backend so the parent repo can safely reference it (ensure git push
origin zmx-termio-backend contains that commit), and rewrite
docs/ghostty-fork.md to replace references to manaflow-ai/ghostty with
mdsakalu/ghostty, clearly describe the fork’s purpose (zmx-termio-backend
support), list the specific changes (e.g., OSC 99 parser/display link restart
behavior if applicable or note they differ), and add any merge-conflict notes
relevant to that branch before re-submitting the PR.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:3721">
P2: Localize the new zmx settings strings; hard-coded English text bypasses the app’s localization system.</violation>
</file>
<file name=".gitmodules">
<violation number="1" location=".gitmodules:3">
P2: Use the organization-maintained Ghostty fork for the submodule URL instead of a personal fork to avoid a brittle build dependency.</violation>
</file>
<file name="Sources/ZmxSupport.swift">
<violation number="1" location="Sources/ZmxSupport.swift:158">
P1: `send(fd, &msg, ...)` is passing the array object address, not the IPC payload bytes, so the zmx kill message can be malformed and ignored.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:4622">
P2: `killZmxSessions()` iterates stale `zmxSessionNames` entries, so workspace close can terminate sessions that no longer belong to active panels.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| path = ghostty | ||
| url = https://github.com/manaflow-ai/ghostty.git | ||
| branch = main | ||
| url = https://github.com/mdsakalu/ghostty.git |
There was a problem hiding this comment.
P2: Use the organization-maintained Ghostty fork for the submodule URL instead of a personal fork to avoid a brittle build dependency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .gitmodules, line 3:
<comment>Use the organization-maintained Ghostty fork for the submodule URL instead of a personal fork to avoid a brittle build dependency.</comment>
<file context>
@@ -1,7 +1,7 @@
path = ghostty
- url = https://github.com/manaflow-ai/ghostty.git
- branch = main
+ url = https://github.com/mdsakalu/ghostty.git
+ branch = zmx-termio-backend
[submodule "homebrew-cmux"]
</file context>
| url = https://github.com/mdsakalu/ghostty.git | |
| url = https://github.com/manaflow-ai/ghostty.git |
There was a problem hiding this comment.
2 issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:2316">
P1: Reacquire `self.surface` after `forceRefreshSurface()` in the retry path; using the previously cached pointer can call Ghostty C APIs on a freed surface.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:2923">
P2: If createTab fails while reattaching a detached surface, the new zmxSessionNames entry survives even though the panel is rolled back. That leaves a stale session mapping that can be persisted or killed on workspace close without any live panel.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
2939-2948:⚠️ Potential issue | 🟡 MinorAdd zmxSessionNames cleanup on createTab failure.
When
createTabfails, the cleanup block removes entries from multiple metadata maps but misseszmxSessionNamesthat was just added at lines 2923-2927. This leaves a stale mapping for a panel that couldn't be attached.🔧 Proposed fix
manualUnreadPanelIds.remove(detached.panelId) manualUnreadMarkedAt.removeValue(forKey: detached.panelId) panelSubscriptions.removeValue(forKey: detached.panelId) + zmxSessionNames.removeValue(forKey: detached.panelId) `#if` DEBUG dlog(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 2939 - 2948, The cleanup branch that runs when createTab fails is removing many panel-related maps (panels, panelDirectories, panelTitles, panelCustomTitles, pinnedPanelIds, manualUnreadPanelIds, manualUnreadMarkedAt, panelSubscriptions) but omits removing the zmxSessionNames entry that was inserted earlier; update the failure/else cleanup block (the same block that removes panels.removeValue(forKey: detached.panelId) etc.) to also remove zmxSessionNames.removeValue(forKey: detached.panelId) (or equivalent removal from zmxSessionNames) so the temporary zmxSessionNames mapping isn’t left stale when createTab fails.
🤖 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 2863-2864: resetAllSettings() currently doesn't clear the new zmx
`@AppStorage` keys, so zmxPersistenceEnabled and zmxKillOnWorkspaceClose keep
their old values after a reset; update resetAllSettings() to explicitly restore
these to their defaults by setting zmxPersistenceEnabled = false and
zmxKillOnWorkspaceClose = true (use the same property names used in
Sources/cmuxApp.swift) as part of the overall settings reset routine so the ZMX
toggles are reset alongside other settings.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2276-2325: The recovery code in runInitialPresentRetry is driving
manual draw loops and reusing a captured surface pointer (currentSurface), which
violates guidance; change runInitialPresentRetry (and related retry flow started
by scheduleInitialPresentRetries/armInitialPresentRetry) to stop calling
ghostty_surface_draw and to not reuse the earlier-captured currentSurface: after
calling view.forceRefreshSurface() (and optionally ghostty_surface_refresh if
needed), re-read self.surface from the instance and only check for success via
view.layer?.contents (or cancel if retries exhausted); do not call
ghostty_surface_draw, do not hold a local currentSurface across the refresh, and
keep the retry scheduling wakeup-based (armInitialPresentRetry) rather than
driving Ghostty directly.
In `@Sources/TabManager.swift`:
- Around line 1226-1235: The early return after calling
AppDelegate.shared.closeMainWindowContainingTabId(_:) assumes the helper always
succeeds; instead detect whether a window-close was actually requested/succeeded
(e.g., change/inspect closeMainWindowContainingTabId to return a Bool or query
AppDelegate for the window existence) and only return when that call indicates
success; otherwise run workspace.teardownAllPanels(), tabs.remove(at: index)
(and any earlier cleanup already done) before returning so the workspace/panels
are removed locally when the delegated close no-ops.
In `@Sources/Workspace.swift`:
- Around line 4627-4632: The method assignZmxSessionName(for:) is unused; either
delete it or document its purpose if intended for external/future use. Remove
the dead function assignZmxSessionName(for panelId: UUID) if there are no
external callers, or replace it with a concise comment explaining it's a public
API/reserved helper and how it differs from direct calls to
zmxAutoAssignSessionName() and direct mutation of zmxSessionNames; reference the
symbols assignZmxSessionName(for:), zmxAutoAssignSessionName(), and
zmxSessionNames when making the change.
In `@Sources/ZmxSupport.swift`:
- Around line 169-194: killSession() is doing blocking connect/write on the main
actor (TabManager) which can stall the UI if the zmx daemon is unreachable; move
the socket I/O off the main thread or implement the same non-blocking
connect+poll pattern used by isSessionAlive() to avoid blocking. Specifically,
update killSession() to either dispatch its socket creation/connect/write/close
logic (the block using socket(AF_UNIX,...), setNoSigPipe(fd:), connecting
sockaddr_un built from socketPath, and writeAll(fd:)) to a background queue or
change the socket to non-blocking (set O_NONBLOCK like isSessionAlive()), use
poll() with a timeout for connect, and only perform final cleanup/any UI updates
back on the main actor; ensure you reuse the same helpers setNoSigPipe(fd:) and
writeAll(fd:) and preserve the header bytes semantics.
- Around line 131-135: The getsockopt call in ZmxSupport.swift currently ignores
its return value, causing soError to remain 0 on failure; update the probe (the
socket health check function containing the getsockopt call) to check the return
value and treat any getsockopt failure as a dead session by returning false when
getsockopt returns -1 (or a non-zero error result), otherwise proceed to
evaluate soError == 0 as before.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 2939-2948: The cleanup branch that runs when createTab fails is
removing many panel-related maps (panels, panelDirectories, panelTitles,
panelCustomTitles, pinnedPanelIds, manualUnreadPanelIds, manualUnreadMarkedAt,
panelSubscriptions) but omits removing the zmxSessionNames entry that was
inserted earlier; update the failure/else cleanup block (the same block that
removes panels.removeValue(forKey: detached.panelId) etc.) to also remove
zmxSessionNames.removeValue(forKey: detached.panelId) (or equivalent removal
from zmxSessionNames) so the temporary zmxSessionNames mapping isn’t left stale
when createTab fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20102d48-7c25-4661-888b-c0cad87cf5f2
📒 Files selected for processing (10)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/Panels/TerminalPanel.swiftSources/TabManager.swiftSources/Workspace.swiftSources/ZmxSupport.swiftSources/cmuxApp.swiftghosttyghostty.h
✅ Files skipped from review due to trivial changes (2)
- ghostty
- Resources/Localizable.xcstrings
mdsakalu
force-pushed
the
push-xvtoqysospul
branch
from
003155a to
1fa0a59
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
2298-2328:⚠️ Potential issue | 🔴 CriticalKeep first-present recovery wakeup-driven; remove direct draw / occlusion pulses.
runInitialPresentRetryandforceRefreshstill callghostty_surface_draw(...), andforceRefreshalso flips occlusion to fake a hide/show. That reintroduces the manual render loop this file explicitly avoids, along with the same attach/reparent surface-lifetime risk the earlier review already called out. Keep recovery toforceRefreshSurface()+ghostty_surface_refresh(...)+ wakeup-based retries only.Run this to confirm the remaining direct-draw recovery paths:
#!/bin/bash rg -n -C3 'ghostty_surface_draw|ghostty_surface_set_occlusion' Sources/GhosttyTerminalView.swiftSuggested change
private func runInitialPresentRetry(reason: String) { initialPresentRetryWorkItem = nil guard portalLifecycleState == .live else { return } @@ initialPresentRetriesRemaining -= 1 view.forceRefreshSurface() guard let currentSurface = surface else { cancelInitialPresentRetries() return } ghostty_surface_refresh(currentSurface) - ghostty_surface_draw(currentSurface) if view.layer?.contents != nil || initialPresentRetriesRemaining <= 0 { cancelInitialPresentRetries() return } @@ view.forceRefreshSurface() guard let surface = self.surface else { return } ghostty_surface_refresh(surface) if view.layer?.contents == nil { - // Freshly attached surfaces can get stuck until a later hide/show cycle - // toggles Ghostty's visible state. Recreate that pulse locally before an - // immediate draw so blank first frames recover without user interaction. - ghostty_surface_set_occlusion(surface, false) - ghostty_surface_set_occlusion(surface, true) - ghostty_surface_draw(surface) scheduleInitialPresentRetries(reason: reason) } else { cancelInitialPresentRetries() } }As per coding guidelines, "Do not add an app-level display link or manual
ghostty_surface_drawloop; rely on Ghostty wakeups/renderer to avoid typing lag".Also applies to: 2767-2777
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 2298 - 2328, runInitialPresentRetry currently calls ghostty_surface_draw and the fix should remove any direct-draw/occlusion touching so recovery is wakeup-driven only: drop the ghostty_surface_draw(...) call(s) and any ghostty_surface_set_occlusion flips in runInitialPresentRetry and in forceRefreshSurface so those functions only call view.forceRefreshSurface() and ghostty_surface_refresh(currentSurface) (and then armInitialPresentRetry or cancelInitialPresentRetries as before); search for the same pattern (ghostty_surface_draw or ghostty_surface_set_occlusion) elsewhere (e.g., the other recovery block mentioned) and remove direct draws there as well, leaving wakeup/renderer-driven retries only.
🧹 Nitpick comments (4)
Sources/cmuxApp.swift (2)
3748-3750: Avoid probing zmx availability frombody.
ZmxSessionProbe.isZmxAvailable()does synchronous filesystem/PATH scanning inSources/ZmxSupport.swift:138-160. Calling it here means every Settings rerender repeats that work on the main thread. Cache the result in state or behind a lazy/static memoized helper and refresh it explicitly only when needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 3748 - 3750, The Settings view is calling ZmxSessionProbe.isZmxAvailable() directly in its body, causing synchronous PATH/filesystem scanning on every render; change this to compute once and cache the result (e.g., store the availability in a `@State/`@StateObject or a static memoized helper) and update it only when needed (for example onAppear or via an explicit refresh action), then use that cached boolean in the conditional that shows SettingsCardDivider and SettingsCardNote; reference ZmxSessionProbe.isZmxAvailable() as the probe function and the Settings UI components (SettingsCardDivider, SettingsCardNote) so the costly probe is removed from the body render path.
3736-3745: Make the dependent toggle read as inactive when persistence is off.When
zmxPersistenceEnabledisfalse, this row can still show the stored “kill on close” subtitle even though the control itself is disabled. Disabling/dimming the whole row, or switching to a dependency-specific subtitle, would make the relationship clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 3736 - 3745, The SettingsCardRow currently shows the stored "kill on close" subtitle even when zmxPersistenceEnabled is false; update the subtitle logic in SettingsCardRow to display a dependency-specific message when !zmxPersistenceEnabled (e.g., "Disabled because persistence is off") or alternatively dim/disable the entire SettingsCardRow UI when zmxPersistenceEnabled is false so the Toggle (bound to zmxKillOnWorkspaceClose) and its subtitle both appear inactive; target the SettingsCardRow instantiation and the Toggle binding (zmxKillOnWorkspaceClose, zmxPersistenceEnabled) to implement the conditional subtitle or apply .disabled(!zmxPersistenceEnabled)/.opacity(...) to the whole row.Sources/ZmxSupport.swift (2)
164-169: Remove redundant assignment.Line 165 creates a new binding with the same name and value as the parameter, which is unnecessary.
🔧 Suggested fix
static func killSession(_ sessionName: String) { - let sessionName = sessionName DispatchQueue.global(qos: .utility).async { killSessionSync(sessionName) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ZmxSupport.swift` around lines 164 - 169, In killSession(_ sessionName: String) remove the redundant local binding "let sessionName = sessionName" so the function uses the parameter directly; keep the DispatchQueue.global(qos: .utility).async { killSessionSync(sessionName) } call unchanged and ensure no other code relied on the shadowed variable name.
10-12: Inconsistent pattern for reading boolean defaults.
isEnabledusesUserDefaults.standard.bool(forKey:)directly, which returnsfalsefor missing keys. Other settings in this codebase (seeWorkspaceAutoReorderSettings.isEnabled, similar patterns incmuxApp.swift) checkobject(forKey:) == nilfirst to distinguish "never set" from "explicitly set to false" and apply a default value.If zmx persistence should default to disabled, the current implementation is correct. However, this differs from
killOnWorkspaceCloseon line 14-16 which properly handles the nil case with a default oftrue.🔧 Suggested fix for consistency
static var isEnabled: Bool { - UserDefaults.standard.bool(forKey: enabledKey) + UserDefaults.standard.object(forKey: enabledKey) as? Bool ?? false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ZmxSupport.swift` around lines 10 - 12, isEnabled currently calls UserDefaults.standard.bool(forKey: enabledKey) which hides "never set" vs "explicitly false"; change it to the same nil-check pattern used by killOnWorkspaceClose/WorkspaceAutoReorderSettings.isEnabled: query UserDefaults.standard.object(forKey: enabledKey) == nil and return the chosen default (explicitly return false when nil) otherwise return UserDefaults.standard.bool(forKey: enabledKey), referencing the isEnabled property and enabledKey to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2298-2328: runInitialPresentRetry currently calls
ghostty_surface_draw and the fix should remove any direct-draw/occlusion
touching so recovery is wakeup-driven only: drop the ghostty_surface_draw(...)
call(s) and any ghostty_surface_set_occlusion flips in runInitialPresentRetry
and in forceRefreshSurface so those functions only call
view.forceRefreshSurface() and ghostty_surface_refresh(currentSurface) (and then
armInitialPresentRetry or cancelInitialPresentRetries as before); search for the
same pattern (ghostty_surface_draw or ghostty_surface_set_occlusion) elsewhere
(e.g., the other recovery block mentioned) and remove direct draws there as
well, leaving wakeup/renderer-driven retries only.
---
Nitpick comments:
In `@Sources/cmuxApp.swift`:
- Around line 3748-3750: The Settings view is calling
ZmxSessionProbe.isZmxAvailable() directly in its body, causing synchronous
PATH/filesystem scanning on every render; change this to compute once and cache
the result (e.g., store the availability in a `@State/`@StateObject or a static
memoized helper) and update it only when needed (for example onAppear or via an
explicit refresh action), then use that cached boolean in the conditional that
shows SettingsCardDivider and SettingsCardNote; reference
ZmxSessionProbe.isZmxAvailable() as the probe function and the Settings UI
components (SettingsCardDivider, SettingsCardNote) so the costly probe is
removed from the body render path.
- Around line 3736-3745: The SettingsCardRow currently shows the stored "kill on
close" subtitle even when zmxPersistenceEnabled is false; update the subtitle
logic in SettingsCardRow to display a dependency-specific message when
!zmxPersistenceEnabled (e.g., "Disabled because persistence is off") or
alternatively dim/disable the entire SettingsCardRow UI when
zmxPersistenceEnabled is false so the Toggle (bound to zmxKillOnWorkspaceClose)
and its subtitle both appear inactive; target the SettingsCardRow instantiation
and the Toggle binding (zmxKillOnWorkspaceClose, zmxPersistenceEnabled) to
implement the conditional subtitle or apply
.disabled(!zmxPersistenceEnabled)/.opacity(...) to the whole row.
In `@Sources/ZmxSupport.swift`:
- Around line 164-169: In killSession(_ sessionName: String) remove the
redundant local binding "let sessionName = sessionName" so the function uses the
parameter directly; keep the DispatchQueue.global(qos: .utility).async {
killSessionSync(sessionName) } call unchanged and ensure no other code relied on
the shadowed variable name.
- Around line 10-12: isEnabled currently calls
UserDefaults.standard.bool(forKey: enabledKey) which hides "never set" vs
"explicitly false"; change it to the same nil-check pattern used by
killOnWorkspaceClose/WorkspaceAutoReorderSettings.isEnabled: query
UserDefaults.standard.object(forKey: enabledKey) == nil and return the chosen
default (explicitly return false when nil) otherwise return
UserDefaults.standard.bool(forKey: enabledKey), referencing the isEnabled
property and enabledKey to locate the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d13d3938-92c2-4061-b186-c474ef25d166
📒 Files selected for processing (8)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/TabManager.swiftSources/Workspace.swiftSources/ZmxSupport.swiftSources/cmuxApp.swiftghosttyghostty.h
✅ Files skipped from review due to trivial changes (1)
- ghostty
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/AppDelegate.swift
mdsakalu
force-pushed
the
push-xvtoqysospul
branch
from
dc986bf to
42d665a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d5aa53ad0
ℹ️ 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".
ghostty
Outdated
| @@ -1 +0,0 @@ | |||
| Subproject commit a50579bd5ddec81c6244b9b349d4bf781f667cec | |||
There was a problem hiding this comment.
Deleting the ghostty gitlink makes fresh checkouts non-buildable: git submodule update --init --recursive will no longer materialize ghostty, but scripts/setup.sh still immediately executes git -C ghostty rev-parse HEAD and later cd ghostty, so setup fails before build. Keep the submodule tracked in the tree and update its SHA instead of removing the gitlink.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bf7ef8 — the ghostty submodule gitlink is now present in all PR commits. The deletion was a JJ submodule snapshotting artifact that leaked into the pushed commit.
…locking killSession - Replace ghostty_surface_draw calls with occlusion toggle + ghostty_surface_refresh (wakeup-based, per coding guidelines) - Use non-blocking connect + poll with 500ms timeout in killSessionSync - Fix Xcode project ID collision between ZmxSupport and AppleScriptSupport
mdsakalu
force-pushed
the
push-xvtoqysospul
branch
from
2d5aa53 to
6bf7ef8
Compare
|
Does this work with ssh sessions oob? |
It should. It just runs terminals in zmx sessions, which are compatible with ssh. If you disconnect from the zmx session, the pty (and thus your ssh connection) would still be running. |
The goal would be for sessions to be persistent on the remote too. But zmx has a current limitation with nested sessions, you can't have zmx on both client and remote.
In either solution, cmux would need to detect that those tabs are remote, and instead of re-attaching the zmx session, use Zmosh is ideal but since there are no iOS zmosh clients yet I wouldn't use it until then, and it's best that cmux just stores the last command that was ran on that tab in the layout metadata and re-runs it. ideally a configurable re-attach mechanism (saving the metadata to support any mechanism). |
3 participants
Summary
This PR implements session persistence in cmux by wiring workspace restore/save to zmx-backed Ghostty surfaces.
At a high level:
Why This Approach
Persistence has two independent concerns:
This implementation intentionally splits them:
That separation keeps each layer focused and avoids embedding process supervision logic into cmux UI state management.
What Changed
1) zmx-backed terminal wiring in cmux
TerminalSurfacenow passeszmx_session/zmx_createinto Ghostty surface config.TerminalPanelconstructors accept zmx session parameters.Enable zmx Session PersistenceKill Sessions on Workspace Close2) Workspace snapshot + restore model
WorkspaceStatePersistence.swift.~/Library/Application Support/cmux/workspace-state.json3) Deterministic zmx session naming + lifecycle
cmux-<stable-id-prefix>-<panel-index>4) Restore behavior and resilience
zmx_create = false)zmx_create = true)5) Mapping consistency fixes
6) Submodule/fork wiring
.gitmodulespoints Ghostty submodule tomdsakalu/ghosttyfork branch containing zmx backend support.ghostty.hsynced with new zmx fields used by cmux.Tests / Validation
cmuxTests/CmuxWebViewKeyEquivalentTests.swiftcovering:xcodebuildWorkspaceZmxPersistenceRegressionTestspassesDependency
Related Issues
Summary by CodeRabbit
New Features
Improvements
Chores