Repository navigation
Conversation
|
@arzafran is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 WalkthroughWalkthroughGuards and best-effort behavior added across CLI hooks and shell integration; conditional session-id injection in the claude wrapper; window-scoped sidebar toggle; TerminalSurface liveness predicate; split-equalization now counts leaves; split-zoom clearing/refinement and portal visibility reconciliation updated. Changes
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)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
Contributor
Greptile SummaryThis community PR addresses 8 open macOS issues (crashes, hook errors, UI bugs, and shell noise) alongside all 7 open Dependabot security alerts. The bulk of the Swift changes are well-targeted crash fixes (
Key changes:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Shell as Bash Shell
participant Wrapper as Resources/bin/claude
participant RealClaude as Real claude binary
participant Hook as cmux claude-hook
participant CLI as CLI/cmux.swift
participant TM as TabManager
participant WS as Workspace
participant Surface as TerminalSurface
Shell->>Wrapper: claude [args]
Wrapper->>RealClaude: --help (capability check)
RealClaude-->>Wrapper: help text
Wrapper->>RealClaude: exec --session-id UUID --settings HOOKS_JSON [args]
RealClaude->>Hook: session-start hook
Hook->>CLI: subcommand=session-start
CLI->>TM: resolveWorkspaceId (throws on failure)
TM-->>CLI: workspaceId
CLI-->>RealClaude: OK
RealClaude->>Hook: notification hook
Hook->>CLI: subcommand=notification
CLI->>TM: resolveWorkspaceId (try?)
alt TabManager live
TM-->>CLI: workspaceId
CLI->>Surface: isSurfaceLive check
Surface-->>CLI: true/false
CLI-->>RealClaude: OK (notify or skip)
else TabManager torn down
CLI-->>RealClaude: OK (graceful no-op)
end
RealClaude->>Hook: stop / session-end hook
Hook->>CLI: subcommand=stop or session-end
CLI->>TM: resolveWorkspaceId (try?)
alt TabManager torn down
CLI-->>RealClaude: OK (graceful no-op)
else TabManager live
CLI->>WS: setClaudeStatus Idle
WS-->>CLI: done
CLI-->>RealClaude: OK
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux.swift (1)
10188-10222:⚠️ Potential issue | 🟠 MajorUse
sendV1Command(...)fornotify_targethere too.Line 10214 still calls
client.send(...), which does not turn plain-textERROR:replies into throws. A teardown race after surface resolution will therefore printERROR: ...instead of the intendedOK.🐛 Proposed fix
- let response = (try? client.send(command: "notify_target \(workspaceId) \(surfaceId) \(payload)")) ?? "OK" + let response = (try? sendV1Command( + "notify_target \(workspaceId) \(surfaceId) \(payload)", + client: client + )) ?? "OK"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 10188 - 10222, The notify_target call uses client.send(...) which doesn't turn plain-text "ERROR:" replies into throws; replace the usage of client.send in the response assignment with the existing sendV1Command(...) helper so ERROR responses become throws and the fallback "OK" behavior on teardown still applies. Specifically, change the line that sets response from (try? client.send(command: "notify_target \(workspaceId) \(surfaceId) \(payload)")) ?? "OK" to use sendV1Command(...) (preserving the same command string and the fallback to "OK"), leaving surrounding logic that calls resolveSurfaceIdForClaudeHook, sessionStore.upsert, and setClaudeStatus unchanged.
🧹 Nitpick comments (1)
Sources/Workspace.swift (1)
7987-7991: Centralize the post-unzoom reconciliation.Line 7989 now routes through
clearSplitZoom(), but the browser/portal/layout follow-up still lives only intoggleSplitZoom(panelId:). Pulling that common work intoclearSplitZoom()will keep the two unzoom paths aligned.♻️ Possible refactor
func moveFocus(direction: NavigationDirection) { // If a pane is zoomed, un-zoom before navigating so the target // pane becomes visible — matches tmux behavior (`#1605`). if bonsplitController.isSplitZoomed { - _ = clearSplitZoom() + _ = clearSplitZoom(reason: "workspace.moveFocus") } @@ `@discardableResult` - func clearSplitZoom() -> Bool { - bonsplitController.clearPaneZoom() + func clearSplitZoom(reason: String = "workspace.clearSplitZoom") -> Bool { + guard bonsplitController.clearPaneZoom() else { return false } + reconcileTerminalPortalVisibilityForCurrentRenderedLayout() + reconcileBrowserPortalVisibilityForCurrentRenderedLayout(reason: reason) + beginEventDrivenLayoutFollowUp(reason: reason, includeGeometry: true) + return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 7987 - 7991, Move the post-unzoom reconciliation (browser/portal/layout updates currently present only in toggleSplitZoom(panelId:)) into clearSplitZoom() so both unzoom code paths perform the same follow-up work; update clearSplitZoom() to perform the browser/portal/layout reconciliation after clearing bonsplitController.isSplitZoomed state and ensure toggleSplitZoom(panelId:) calls clearSplitZoom() (removing its duplicated reconciliation code) so there is a single, centralized unzoom behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 10037-10053: The current catch around
resolveWorkspaceIdForClaudeHook lets it fall back to workspace.current and thus
can pick a dead/unrelated workspace; change resolution to be strict/optional:
call resolveWorkspaceIdForClaudeHook in a mode that does NOT fall back to
workspace.current (e.g., add/pass allowFallback: false or use a new
resolveWorkspaceIdStrict helper) so it throws when the Claude workspace is gone,
and only treat missing workspace as non-fatal for subcommand values other than
"session-start" and "active"; if no workspace is resolved then skip downstream
calls that operate on the workspace (clear_notifications, setClaudeStatus,
resolveSurfaceIdForClaudeHook) and ensure session-end still proceeds to
sessionStore.consume when appropriate.
In `@Resources/shell-integration/cmux-bash-integration.bash`:
- Around line 102-106: The subshell invocations that end with ) & still create
parent-shell jobs; change fire-and-forget senders that call _cmux_send (and
similar helpers) to run an inner backgrounded command inside a foreground
subshell, e.g. ( command & ) >/dev/null 2>&1 to prevent the interactive shell
from tracking them, and for long-lived pollers (the poller/gits-status async
blocks) capture the PID with pid=$! immediately after starting and run disown
"$pid" so the job is removed from the shell job table; apply this pattern to all
similar blocks (the _cmux_send callers and the git-status async work) referenced
in the diff.
In `@Sources/AppDelegate.swift`:
- Around line 5357-5361: The code currently uses contextForMainWindow(window)
which can miss a stale ObjectIdentifier map and then falls back to
toggleSidebarInActiveMainWindow() even when a specific window was supplied;
replace that lookup with contextForMainTerminalWindow(window) (which can
reindex) and if it returns non-nil call sidebarState.toggle() and return true,
but do not fall back to toggleSidebarInActiveMainWindow() when the original
window parameter was non-nil—only call the active-window fallback when the
incoming window argument itself is nil.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 10188-10222: The notify_target call uses client.send(...) which
doesn't turn plain-text "ERROR:" replies into throws; replace the usage of
client.send in the response assignment with the existing sendV1Command(...)
helper so ERROR responses become throws and the fallback "OK" behavior on
teardown still applies. Specifically, change the line that sets response from
(try? client.send(command: "notify_target \(workspaceId) \(surfaceId)
\(payload)")) ?? "OK" to use sendV1Command(...) (preserving the same command
string and the fallback to "OK"), leaving surrounding logic that calls
resolveSurfaceIdForClaudeHook, sessionStore.upsert, and setClaudeStatus
unchanged.
---
Nitpick comments:
In `@Sources/Workspace.swift`:
- Around line 7987-7991: Move the post-unzoom reconciliation
(browser/portal/layout updates currently present only in
toggleSplitZoom(panelId:)) into clearSplitZoom() so both unzoom code paths
perform the same follow-up work; update clearSplitZoom() to perform the
browser/portal/layout reconciliation after clearing
bonsplitController.isSplitZoomed state and ensure toggleSplitZoom(panelId:)
calls clearSplitZoom() (removing its duplicated reconciliation code) so there is
a single, centralized unzoom behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7516144-c490-40f1-ae29-8e1649a8c7b7
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
CLI/cmux.swiftResources/bin/claudeResources/shell-integration/cmux-bash-integration.bashSources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/TabManager.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/Workspace.swiftSources/cmuxApp.swiftweb/package.json
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 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="Resources/shell-integration/cmux-bash-integration.bash">
<violation number="1" location="Resources/shell-integration/cmux-bash-integration.bash:103">
P2: Incorrect Bash job-control assumption removed `disown`, which can reintroduce background job completion/kill notifications (prompt spam).</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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Workspace.swift`:
- Around line 8062-8067: clearSplitZoom currently clears pane zoom without
priming a browser portal host replacement, which lets a zoomed browser remain
attached to a stale host; update clearSplitZoom to call the same priming helper
used by toggleSplitZoom — preparePortalHostReplacementForNextDistinctClaim(...)
— before or immediately after bonsplitController.clearPaneZoom() (and before
reconcileBrowserPortalVisibilityForCurrentRenderedLayout) so the browser portal
host is replaced for the next distinct claim; reference clearSplitZoom,
bonsplitController.clearPaneZoom,
preparePortalHostReplacementForNextDistinctClaim, toggleSplitZoom, and
reconcileBrowserPortalVisibilityForCurrentRenderedLayout when applying the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9079db27-26cb-455d-b609-bdf1a1420d7c
📒 Files selected for processing (4)
CLI/cmux.swiftResources/bin/claudeSources/AppDelegate.swiftSources/Workspace.swift
✅ Files skipped from review due to trivial changes (1)
- CLI/cmux.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Resources/bin/claude
… bash notifications
- Guard dangling ghostty_surface_t with isSurfaceLive before C API calls (#1558, #1608, #1767, #1815)
- Make CLI lifecycle hooks degrade gracefully when TabManager is torn down (#1775, #1715)
- Claude shim checks --session-id support before using it (#1818)
- Sidebar toggle targets its own window via WeakSelfBox pattern (#1779)
- Un-zoom pane before focus navigation, matching tmux behavior (#1605)
- Replace { } & disown with subshell ( ) & to suppress bash job notifications (#1565)
- Equalize splits uses proportional leaf counting for nested same-axis panes (#1622)
- Move Ghostty Settings below Settings in app menu (#1680)
Update next 16.1.6→16.1.7 (HTTP smuggling, CSRF bypass, DoS), flatted 3.3.3→3.4.2 (prototype pollution), dompurify 3.3.1→3.3.3 (XSS), minimatch 3.1.2→3.1.5 and 9.0.5→9.0.9 (ReDoS).
( ... ) & still creates a job in bash's job table — it is the & operator that adds jobs, not the command type. Without disown, completed background jobs produce "[N]+ Done" notifications at the next prompt. Use subshell for process isolation + disown to suppress job-table tracking (#1565).
…ation - Cache --session-id capability check per binary path+mtime to avoid re-running --help on every invocation (greptile P2) - Fix comment to mention both session-start and active require a live workspace (greptile P2) - Use sendV1Command instead of client.send for notify_target so ERROR replies become throws with proper fallback (coderabbit) - Make workspace resolution strict: when a specific workspace ID was provided but the workspace is gone, throw instead of falling back to workspace.current which could target an unrelated workspace (coderabbit) - Use contextForMainTerminalWindow (reindex-capable) for sidebar toggle; only fall back to active window when no target window provided (coderabbit) - Centralize post-unzoom portal/layout reconciliation in clearSplitZoom() so moveFocus un-zoom path matches toggleSplitZoom behavior (coderabbit)
When un-zooming via clearSplitZoom (e.g. from moveFocus), capture the zoomed pane's browser panel before clearing zoom and call preparePortalHostReplacementForNextDistinctClaim so the browser portal doesn't stay attached to the stale zoom host (coderabbit review).
arzafran
force-pushed
the
fix/macos-issues-and-deps
branch
from
March 20, 2026 11:58
e26b8ac to
2e8222a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
CLI/cmux.swift (3)
10139-10155:⚠️ Potential issue | 🟠 MajorDon't short-circuit
session-endbefore consuming the stored session.Lines 10150-10152 put
session-endin the silent early-return bucket, so when the workspace is already gone we printOKbefore reachingsessionStore.consume(...)on Lines 10332-10336. That leaves stale Claude hook state behind instead of cleaning it up. Missing workspace should suppress the later tab/surface mutations, butsession-endstill needs to consume the session first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 10139 - 10155, The current early-return on failed resolveWorkspaceIdForClaudeHook short-circuits "session-end" and prevents sessionStore.consume(...) from running; update the conditional so "session-end" is NOT included in the silent-return list (only exclude "session-start" and "active"), allowing the session-end path to proceed to sessionStore.consume (see sessionStore.consume usage) while ensuring subsequent tab/surface mutation code checks for a valid fallbackWorkspaceId (or guards when workspace resolution failed) before performing workspace mutations.
10227-10239:⚠️ Potential issue | 🟠 MajorThese best-effort guards can still notify the wrong surface.
The new
try?/guardlogic only no-ops ifresolveSurfaceIdForClaudeHook(...)throws, but that helper ultimately relies on the permissive resolver on Lines 5907-5935. A stale ref/index surface handle still falls back to the workspace's focused surface, sostop/notificationcan land on an unrelated tab instead of quietly returningOK.Also applies to: 10290-10298
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 10227 - 10239, The current best-effort block uses resolveSurfaceIdForClaudeHook(...) which may silently fall back to the workspace's focused surface and cause notifications to land on unrelated tabs; change the logic so we only notify when the resolver returns a true match for the original surface handle instead of a permissive fallback: either add/use a strict resolver variant (e.g., resolveSurfaceIdForClaudeHookStrict) that returns nil for stale handles, or have resolveSurfaceIdForClaudeHook expose whether the result is a fallback and bail if so; apply this same guard where sendV1Command("notify_target ...", client:) is called (both the shown block and the duplicate at 10290-10298) so we never send a notification when the input surfaceId cannot be resolved exactly.
10519-10530:⚠️ Potential issue | 🟠 Major
resolveWorkspaceIdForClaudeHookis still not strict for every explicit target.This helper still builds on
resolveWorkspaceId(...), and that generic resolver on Lines 5874-5905 falls through toworkspace.currentfor non-UUID/non-ref/non-index input. On top of that, thetry?here turns stale ref/index misses intonil, which also falls through toworkspace.current. So some bad explicit--workspace/CMUX_WORKSPACE_IDvalues will still target an unrelated live workspace and bypass thedo/catchabove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 10519 - 10530, resolveWorkspaceIdForClaudeHook currently lets explicit raw inputs fall back to resolveWorkspaceId(nil,...)/workspace.current because it uses try? and the generic resolveWorkspaceId accepts non-UUID/non-ref/non-index input; change it so any explicit raw value is validated strictly: call resolveWorkspaceId(raw, client: client) without try? (or add a strict flag to resolveWorkspaceId) and propagate errors instead of swallowing them, only use client.sendV2 probe after a successful resolution, and if resolution fails throw CLIError("Workspace no longer available: \(raw)") (referencing resolveWorkspaceIdForClaudeHook, resolveWorkspaceId, client.sendV2 and CLIError).Resources/shell-integration/cmux-bash-integration.bash (1)
102-107:⚠️ Potential issue | 🟠 MajorThe fire-and-forget helpers are still parent-shell jobs.
Each of these blocks backgrounds the subshell itself, so Bash still creates a job in the interactive shell and only removes it afterward via
disown. That can still leak job-control noise here, which is the exact behavior this change is trying to eliminate. For the one-shot senders, keep the subshell foregrounded and put the&on the inner_cmux_sendinstead.Representative fix
- ( - _cmux_send "report_tty $_CMUX_TTY_NAME --tab=$CMUX_TAB_ID --panel=$CMUX_PANEL_ID" - ) >/dev/null 2>&1 & - disown 2>/dev/null + ( + _cmux_send "report_tty $_CMUX_TTY_NAME --tab=$CMUX_TAB_ID --panel=$CMUX_PANEL_ID" >/dev/null 2>&1 & + )Apply the same shape to the
report_shell_state,ports_kick, andreport_pwdblocks.In interactive Bash with job control enabled, does `( command ) &` create a job in the parent shell's job table, and does `( command & )` avoid putting the inner command in the parent shell's job table? Please answer from the Bash manual sections on asynchronous commands and job control.Also applies to: 118-121, 131-134, 411-415
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/cmux-bash-integration.bash` around lines 102 - 107, The subshells like the one wrapping _cmux_send for report_tty (and the similar report_shell_state, ports_kick, report_pwd blocks) are being backgrounded with `( ... ) &` which creates a parent-shell job; instead, background the inner _cmux_send so the subshell runs in the foreground and no job is recorded: replace the pattern `( _cmux_send "..." ) >/dev/null 2>&1 & disown` with `( _cmux_send "..." & ) >/dev/null 2>&1` (remove the disown) for the report_tty, report_shell_state, ports_kick, and report_pwd blocks.
🧹 Nitpick comments (2)
Sources/TabManager.swift (1)
3345-3361: Add a regression test for the new axis-counting rule.This helper now encodes a subtle invariant: same-axis descendants expand into their leaves, while perpendicular descendants collapse to one unit. A focused case for
A | (B | C)and one perpendicular-child layout would make future tree-shape/orientation changes much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 3345 - 3361, Add a regression test that exercises the countLeaves(in:along:) logic in TabManager by constructing ExternalTreeNode trees for both same-axis nesting and perpendicular-child cases: (1) a horizontal split A | (B | C) where countLeaves(in: , along: "horizontal") should expand to 2 leaves for the nested same-axis child, and (2) a layout where a split's child is perpendicular (e.g. A | (B — C) or similar) where countLeaves(in: , along: "horizontal") should return 1 for the perpendicular subtree; locate and call the private countLeaves(in:along:) helper (or test via a public wrapper if needed) and assert the expected integer results using your test framework so future changes to ExternalTreeNode/split orientation handling are covered.Sources/Workspace.swift (1)
8092-8101: Keep the unzoom exit sequence in one place.
toggleSplitZoom(panelId:)still has its own reconcile + portal-preparation flow, whileclearSplitZoom(reason:)now owns the same sequence for other callers. Pulling the unzoom branch onto the same helper path would reduce the odds of these two flows drifting again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 8092 - 8101, The unzoom exit sequence currently duplicates logic: inside toggleSplitZoom(panelId:) it calls reconcileTerminalPortalVisibilityForCurrentRenderedLayout(), reconcileBrowserPortalVisibilityForCurrentRenderedLayout(reason:), and invokes browserPanel(for:).preparePortalHostReplacementForNextDistinctClaim(...), while clearSplitZoom(reason:) now centralizes this flow; refactor toggleSplitZoom(panelId:) to call clearSplitZoom(reason: "workspace.toggleSplitZoom") for the unzoom branch instead of repeating the reconcile and portal-prep calls so all callers use the single helper, leaving clearSplitZoom(reason:) responsible for calling reconcileTerminalPortalVisibilityForCurrentRenderedLayout(), reconcileBrowserPortalVisibilityForCurrentRenderedLayout(reason:), and browserPanel(for:).preparePortalHostReplacementForNextDistinctClaim(inPane:paneId, reason:).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 10139-10155: The current early-return on failed
resolveWorkspaceIdForClaudeHook short-circuits "session-end" and prevents
sessionStore.consume(...) from running; update the conditional so "session-end"
is NOT included in the silent-return list (only exclude "session-start" and
"active"), allowing the session-end path to proceed to sessionStore.consume (see
sessionStore.consume usage) while ensuring subsequent tab/surface mutation code
checks for a valid fallbackWorkspaceId (or guards when workspace resolution
failed) before performing workspace mutations.
- Around line 10227-10239: The current best-effort block uses
resolveSurfaceIdForClaudeHook(...) which may silently fall back to the
workspace's focused surface and cause notifications to land on unrelated tabs;
change the logic so we only notify when the resolver returns a true match for
the original surface handle instead of a permissive fallback: either add/use a
strict resolver variant (e.g., resolveSurfaceIdForClaudeHookStrict) that returns
nil for stale handles, or have resolveSurfaceIdForClaudeHook expose whether the
result is a fallback and bail if so; apply this same guard where
sendV1Command("notify_target ...", client:) is called (both the shown block and
the duplicate at 10290-10298) so we never send a notification when the input
surfaceId cannot be resolved exactly.
- Around line 10519-10530: resolveWorkspaceIdForClaudeHook currently lets
explicit raw inputs fall back to resolveWorkspaceId(nil,...)/workspace.current
because it uses try? and the generic resolveWorkspaceId accepts
non-UUID/non-ref/non-index input; change it so any explicit raw value is
validated strictly: call resolveWorkspaceId(raw, client: client) without try?
(or add a strict flag to resolveWorkspaceId) and propagate errors instead of
swallowing them, only use client.sendV2 probe after a successful resolution, and
if resolution fails throw CLIError("Workspace no longer available: \(raw)")
(referencing resolveWorkspaceIdForClaudeHook, resolveWorkspaceId, client.sendV2
and CLIError).
In `@Resources/shell-integration/cmux-bash-integration.bash`:
- Around line 102-107: The subshells like the one wrapping _cmux_send for
report_tty (and the similar report_shell_state, ports_kick, report_pwd blocks)
are being backgrounded with `( ... ) &` which creates a parent-shell job;
instead, background the inner _cmux_send so the subshell runs in the foreground
and no job is recorded: replace the pattern `( _cmux_send "..." ) >/dev/null
2>&1 & disown` with `( _cmux_send "..." & ) >/dev/null 2>&1` (remove the disown)
for the report_tty, report_shell_state, ports_kick, and report_pwd blocks.
---
Nitpick comments:
In `@Sources/TabManager.swift`:
- Around line 3345-3361: Add a regression test that exercises the
countLeaves(in:along:) logic in TabManager by constructing ExternalTreeNode
trees for both same-axis nesting and perpendicular-child cases: (1) a horizontal
split A | (B | C) where countLeaves(in: , along: "horizontal") should expand to
2 leaves for the nested same-axis child, and (2) a layout where a split's child
is perpendicular (e.g. A | (B — C) or similar) where countLeaves(in: , along:
"horizontal") should return 1 for the perpendicular subtree; locate and call the
private countLeaves(in:along:) helper (or test via a public wrapper if needed)
and assert the expected integer results using your test framework so future
changes to ExternalTreeNode/split orientation handling are covered.
In `@Sources/Workspace.swift`:
- Around line 8092-8101: The unzoom exit sequence currently duplicates logic:
inside toggleSplitZoom(panelId:) it calls
reconcileTerminalPortalVisibilityForCurrentRenderedLayout(),
reconcileBrowserPortalVisibilityForCurrentRenderedLayout(reason:), and invokes
browserPanel(for:).preparePortalHostReplacementForNextDistinctClaim(...), while
clearSplitZoom(reason:) now centralizes this flow; refactor
toggleSplitZoom(panelId:) to call clearSplitZoom(reason:
"workspace.toggleSplitZoom") for the unzoom branch instead of repeating the
reconcile and portal-prep calls so all callers use the single helper, leaving
clearSplitZoom(reason:) responsible for calling
reconcileTerminalPortalVisibilityForCurrentRenderedLayout(),
reconcileBrowserPortalVisibilityForCurrentRenderedLayout(reason:), and
browserPanel(for:).preparePortalHostReplacementForNextDistinctClaim(inPane:paneId,
reason:).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5085b41-cbed-484f-9fb4-16467aa88b0a
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
CLI/cmux.swiftResources/bin/claudeResources/shell-integration/cmux-bash-integration.bashSources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/TabManager.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/Workspace.swiftSources/cmuxApp.swiftweb/package.json
✅ Files skipped from review due to trivial changes (2)
- web/package.json
- Sources/AppDelegate.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/Update/UpdateTitlebarAccessory.swift
- Resources/bin/claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Community contribution addressing 8 open macOS issues and all 7 Dependabot security alerts.
Bug Fixes
SIGSEGV crash in
inheritedTerminalConfig(#1558, #1608, #1767, #1815)isSurfaceLiveguard onTerminalSurfacethat checks both non-nil pointer AND live portal lifecycle state before passing the surface to any Ghostty C API. Prevents use-after-free when the surface is in async teardown."TabManager not available" hook errors (#1775, #1715)
stop,session-end,notification,prompt-submit) now degrade gracefully when TabManager is already torn down during app/workspace close. Onlysession-startrequires a live workspace.CLI shim
--session-iderror in fresh tabs (#1818)claudewrapper now verifies the resolved binary supports--session-idvia--helpbefore using it. Falls back to hooks-only mode if the flag isn't supported (e.g., stale nvm-resolved binary).Side panel toggle affects wrong window (#1779)
toggleSidebarForWindow(_:)toAppDelegate. Titlebar accessory buttons now capture their ownview.windowvia aWeakSelfBoxpattern, targeting the correct window instead of whichever happens to be key.Zoomed pane doesn't un-zoom on focus navigation (#1605)
moveFocus(direction:)now callsclearSplitZoom()when a pane is zoomed, matching tmux behavior.Bash job termination notification spam (#1565)
{ ... } & disownwith( ... ) &for all background telemetry commands. Subshells are never added to bash's job table, eliminating spurious "Done" notifications.Equalize Splits produces uneven layouts (#1622)
countLeaves(in:along:)to calculate proportional divider positions based on leaf pane count per axis.A | (B | C)now produces 33/33/33 instead of 50/25/25.Menu order — Ghostty Settings above Settings (#1680)
.appInfointo.appSettings, placing them after the Preferences item.Security (Dependabot)
Resolves all 7 open Dependabot alerts (3 high, 3 moderate, 1 low):
nextflatteddompurifyminimatchminimatch(nested)Files Changed
Sources/GhosttyTerminalView.swiftisSurfaceLivecomputed propertySources/Workspace.swiftCLI/cmux.swiftResources/bin/claude--session-idcompatibility checkSources/AppDelegate.swifttoggleSidebarForWindow(_:)Sources/Update/UpdateTitlebarAccessory.swiftSources/TabManager.swiftSources/cmuxApp.swiftResources/shell-integration/cmux-bash-integration.bashweb/package.jsonweb/package-lock.jsonSummary by cubic
Fixes eight macOS bugs to improve stability and window behavior, and closes all Dependabot security alerts by upgrading vulnerable web dependencies.
Bug Fixes
isSurfaceLive.session-startandactiverequire a live workspace. No fallback to other workspaces, and notifications/status updates are best‑effort.claudewrapper verifies--session-idsupport via--help, caches the check per binary path+mtime, and falls back to hooks‑only mode if unsupported.toggleSidebarForWindow(_:)), not the key window.Dependencies
next16.1.6 → 16.1.7flatted3.3.3 → 3.4.2dompurify3.3.1 → 3.3.3minimatch3.1.2 → 3.1.5 and 9.0.5 → 9.0.9eslint-config-nextto16.1.7; lockfile updated with safe transitive versions.Written for commit 2e8222a. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features
Chores