Repository navigation
Reduce workspace and pane geometry churn - #1771
Closed
gaelic-ghost wants to merge 4 commits into
Closed
gaelic-ghost wants to merge 4 commits into
gaelic-ghost wants to merge 4 commits into
Conversation
|
@gaelic-ghost is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
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 epoch-and-coverage-based coalescing for portal geometry/full-syncs, per-entry idempotent sync outcomes, reworks external-geometry vs. deferred full-sync scheduling and reconciliation flow, and expands DEBUG/UI-test tracing and workspace geometry follow-up logic. Public APIs unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant Portal as TerminalWindowPortal
participant Registry as TerminalWindowPortalRegistry
participant WS as WorkspaceScheduler
participant Host as HostWindow/HostView
UI->>Portal: anchor/geometry change or bind
Portal->>Portal: compute FullSyncCoverage & SyncOutcome
alt coverage satisfied
Portal-->>UI: skip scheduling
else need sync
Portal->>Registry: scheduleExternalGeometrySynchronizeForAllWindows()
Registry->>Portal: portal.scheduleExternalGeometrySynchronize()
Portal->>WS: scheduleExternalGeometrySynchronize(coverage, force?)
WS->>WS: coalesce/queue by coverage (syncCoalescingEpoch)
WS->>Host: execute external-geometry/full-sync pass
Host-->>Portal: host frame/bounds and reconciliation result
Portal->>Portal: compute HostedSyncResult -> SyncOutcome
alt settled
Portal->>Portal: mark coverage satisfied
end
Portal-->>UI: apply hosted-view updates or skip if approx-equal
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 0da40304c1
ℹ️ 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".
![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 2 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/TerminalWindowPortal.swift">
<violation number="1" location="Sources/TerminalWindowPortal.swift:796">
P1: `requestedCoverage` is captured before the deferred second-dispatch sync runs, so newer queued geometry coverage can be cleared and skipped.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
🧹 Nitpick comments (1)
Sources/TerminalWindowPortal.swift (1)
2067-2076: Prefer the per-portal scheduler inscheduleExternalGeometrySynchronizeForAllWindows().Calling
synchronizeAllEntriesFromExternalGeometryChange()directly bypasses each portal’s queue/satisfied bookkeeping. Routing throughscheduleExternalGeometrySynchronize()keeps coalescing behavior consistent.Refactor sketch
let performSync = { Self.hasPendingExternalGeometrySyncForAllWindows = false `#if` DEBUG @@ for portal in Self.portalsByWindowId.values { - portal.synchronizeAllEntriesFromExternalGeometryChange() + portal.scheduleExternalGeometrySynchronize() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalWindowPortal.swift` around lines 2067 - 2076, The code directly calls synchronizeAllEntriesFromExternalGeometryChange() on each Portal inside the DispatchQueue.main.async block, which bypasses per-portal scheduling/coalescing; change the loop to call scheduleExternalGeometrySynchronize() for each portal so each portal’s internal queue and satisfied bookkeeping are used. Keep the surrounding logic that clears Self.hasPendingExternalGeometrySyncForAllWindows and the DEBUG logging, but replace the direct call to portal.synchronizeAllEntriesFromExternalGeometryChange() with portal.scheduleExternalGeometrySynchronize() for every entry in Self.portalsByWindowId.values so the per-portal scheduler is respected.
🤖 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/TerminalWindowPortal.swift`:
- Around line 1306-1331: When coalescing deferred full-syncs we currently only
update deferredFullSyncQueuedCoverage and lose a true `force` if a non-forced
call was queued; update the logic to also track and update a
deferredFullSyncQueuedForce (or similar) boolean whenever
hasDeferredFullSyncScheduled is true so the later DispatchQueue.main.async
closure uses the coalesced requestedForce alongside requestedCoverage; inside
the closure read let requestedForce = self.deferredFullSyncQueuedForce ?? force,
clear the stored flag when resetting
hasDeferredFullSyncScheduled/deferredFullSyncQueuedCoverage, and use
requestedForce in the hasSatisfiedFullSync(...) check and subsequent behavior to
ensure forced runs are not dropped (referencing hasDeferredFullSyncScheduled,
deferredFullSyncQueuedCoverage, the force parameter, and
hasSatisfiedFullSync(for:)).
- Around line 638-641: The full-sync gate implemented by
fullSyncCoverageSatisfies only compares epoch, hostFrame and hostBounds so
split-divider moves are being skipped; update the logic and state used by
scheduleExternalGeometrySynchronize() so divider/anchor changes can't be
missed—either extend FullSyncCoverage to include per-anchor divider positions
(or a dividerEpoch) and compare those in fullSyncCoverageSatisfies, or add a
separate quick check triggered from NSSplitView.didResizeSubviewsNotification
that forces synchronizeAllHostedViews() (or calls synchronizeForAnchor() for the
moved anchor) when dividers change; reference FullSyncCoverage,
fullSyncCoverageSatisfies(_:_:), scheduleExternalGeometrySynchronize(),
synchronizeAllHostedViews(), synchronizeForAnchor(), and the
NSSplitView.didResizeSubviewsNotification handler when making the change.
In `@Sources/Workspace.swift`:
- Around line 8516-8558: The fallback DispatchWorkItem currently sets
layoutFollowUpSuppressNextDeferredGeometryPassGeneration (in the workItem
created near layoutFollowUpDeferredBonsplitGeometryFallbackWorkItem), which can
suppress the next real didChangeGeometry callback and drop the first real
update; remove the line that assigns
layoutFollowUpSuppressNextDeferredGeometryPassGeneration = generation from the
fallback workItem body and instead let the existing follow-up coalescing
(layoutFollowUpDeferredBonsplitGeometryGeneration and
layoutFollowUpDeferredBonsplitGeometryReason) handle duplicate events; keep the
rest of the fallback logic (clearing
layoutFollowUpDeferredBonsplitGeometryReason, clearing the fallback work item,
and calling scheduleTerminalGeometryReconcile(reason: "\(reason).fallback"))
unchanged.
---
Nitpick comments:
In `@Sources/TerminalWindowPortal.swift`:
- Around line 2067-2076: The code directly calls
synchronizeAllEntriesFromExternalGeometryChange() on each Portal inside the
DispatchQueue.main.async block, which bypasses per-portal scheduling/coalescing;
change the loop to call scheduleExternalGeometrySynchronize() for each portal so
each portal’s internal queue and satisfied bookkeeping are used. Keep the
surrounding logic that clears Self.hasPendingExternalGeometrySyncForAllWindows
and the DEBUG logging, but replace the direct call to
portal.synchronizeAllEntriesFromExternalGeometryChange() with
portal.scheduleExternalGeometrySynchronize() for every entry in
Self.portalsByWindowId.values so the per-portal scheduler is respected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6aa7760b-05a6-4011-a067-904d121d9443
📒 Files selected for processing (2)
Sources/TerminalWindowPortal.swiftSources/Workspace.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a2886fbd1
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/Workspace.swift (1)
8522-8525:⚠️ Potential issue | 🟠 MajorDon't suppress the first real Bonsplit geometry callback after fallback.
Once the fallback fires, this assignment clears the deferred reason and makes the next same-generation
didChangeGeometryhit the skip path at Lines 10158-10162. If fallback ran before Bonsplit fully settled, the real geometry callback gets dropped and stale bounds can stick around until some unrelated trigger retriggers geometry.♻️ Proposed fix
layoutFollowUpDeferredBonsplitGeometryReason = nil layoutFollowUpDeferredBonsplitGeometryFallbackWorkItem = nil - layoutFollowUpSuppressNextDeferredGeometryPassGeneration = generation `#if` DEBUG dlog( "geometry.defer.fallback ws=\(self.id.uuidString.prefix(5)) reason=\(reason) generation=\(generation)" ) `#endif`Also applies to: 10158-10162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 8522 - 8525, When the Bonsplit fallback runs, don't clear the deferred reason or mark the next same-generation geometry pass as suppressed — that causes the real didChangeGeometry callback to take the skip path and drop the real geometry update. In the fallback path that currently assigns layoutFollowUpDeferredBonsplitGeometryReason = nil and layoutFollowUpSuppressNextDeferredGeometryPassGeneration = generation, remove those assignments and only clear layoutFollowUpDeferredBonsplitGeometryFallbackWorkItem; ensure didChangeGeometry remains responsible for clearing layoutFollowUpDeferredBonsplitGeometryReason and for handling suppression logic so the first real Bonsplit geometry callback after fallback is not skipped.
🧹 Nitpick comments (1)
Sources/TerminalWindowPortal.swift (1)
1186-1219: Preserve sync state on idempotent rebinds.
bindalways bumpssyncCoalescingEpochand recreates the entry withlastSynchronizedOutcome = nil, even when the hosted view is already bound to the same anchor with the same visibility/z-order. That guarantees a deferred full-sync after a no-op rebind and disables the newreason=unchangedfast path for the bind-triggered pass.Also applies to: 1288-1290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalWindowPortal.swift` around lines 1186 - 1219, The bind implementation is clearing sync state and forcing a full sync on no-op rebinds; change it to detect an idempotent rebind by comparing the existing Entry in entriesByHostedId (oldEntry) for the hostedId: if oldEntry.anchorView === anchorView and oldEntry.visibleInUI == visibleInUI and oldEntry.zPriority == zPriority then treat this as idempotent and do not increment syncCoalescingEpoch, do not replace the Entry, and preserve lastSynchronizedOutcome (and transientRecoveryRetriesRemaining); otherwise proceed with the current replacement logic (updating hostedByAnchorId, entriesByHostedId with a new Entry and resetting lastSynchronizedOutcome). Ensure you reference bind, entriesByHostedId, hostedByAnchorId, Entry, lastSynchronizedOutcome, syncCoalescingEpoch, and markPortalStateMutated when making the change.
🤖 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/TerminalWindowPortal.swift`:
- Around line 605-610: FullSyncCoverage currently only tracks epoch, hostFrame,
hostBounds, and anchorLayoutSignature which lets hasSatisfiedFullSync()
incorrectly skip reattachment when a view is remounted at the same geometry;
include the install target identifier (the view/container or installation target
used by ensureInstalled()) in FullSyncCoverage and in all places that
construct/compare it (references near FullSyncCoverage, uses in
hasSatisfiedFullSync, and where coverage is created/updated before/after
ensureInstalled()) so the coverage key differs when the install target changes,
forcing reattachment when remounts occur.
- Around line 840-841: The calls to markFullSyncSatisfied() must only run when
the sync actually completed; update the call sites that call
synchronizeAllEntriesFromExternalGeometryChange() and
synchronizeAllHostedViews() so they first confirm the sync body ran (e.g., check
ensureInstalled() succeeded or make the sync methods return a Bool/Result
indicating they performed work) and only call markFullSyncSatisfied() when that
result is positive; reference synchronizeAllEntriesFromExternalGeometryChange(),
synchronizeAllHostedViews(), ensureInstalled(), and markFullSyncSatisfied() and
ensure you stop recording lastSatisfiedFullSyncCoverage when the sync functions
bail early.
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 8522-8525: When the Bonsplit fallback runs, don't clear the
deferred reason or mark the next same-generation geometry pass as suppressed —
that causes the real didChangeGeometry callback to take the skip path and drop
the real geometry update. In the fallback path that currently assigns
layoutFollowUpDeferredBonsplitGeometryReason = nil and
layoutFollowUpSuppressNextDeferredGeometryPassGeneration = generation, remove
those assignments and only clear
layoutFollowUpDeferredBonsplitGeometryFallbackWorkItem; ensure didChangeGeometry
remains responsible for clearing layoutFollowUpDeferredBonsplitGeometryReason
and for handling suppression logic so the first real Bonsplit geometry callback
after fallback is not skipped.
---
Nitpick comments:
In `@Sources/TerminalWindowPortal.swift`:
- Around line 1186-1219: The bind implementation is clearing sync state and
forcing a full sync on no-op rebinds; change it to detect an idempotent rebind
by comparing the existing Entry in entriesByHostedId (oldEntry) for the
hostedId: if oldEntry.anchorView === anchorView and oldEntry.visibleInUI ==
visibleInUI and oldEntry.zPriority == zPriority then treat this as idempotent
and do not increment syncCoalescingEpoch, do not replace the Entry, and preserve
lastSynchronizedOutcome (and transientRecoveryRetriesRemaining); otherwise
proceed with the current replacement logic (updating hostedByAnchorId,
entriesByHostedId with a new Entry and resetting lastSynchronizedOutcome).
Ensure you reference bind, entriesByHostedId, hostedByAnchorId, Entry,
lastSynchronizedOutcome, syncCoalescingEpoch, and markPortalStateMutated when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afe826a9-3571-4b5d-a141-4441de75964e
📒 Files selected for processing (2)
Sources/TerminalWindowPortal.swiftSources/Workspace.swift
There was a problem hiding this comment.
2 issues found across 2 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/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:8780">
P1: Release builds disable geometry reattach/refresh decisions because non-DEBUG helper stubs always return empty arrays, turning runtime predicates permanently false.</violation>
<violation number="2" location="Sources/Workspace.swift:8788">
P1: Avoid returning a constant empty refresh-reason set in non-DEBUG builds. This can suppress production refresh triggering after layout mutations and leave visible terminal content stale.</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.
🧹 Nitpick comments (1)
Sources/Workspace.swift (1)
8517-8544: Make the deferred fallback delay a named, tunable constant.Line 8543 uses a hardcoded
0.02. Promoting this to a constant (optionally DEBUG-tunable) will make timing calibration safer across different hardware/workspace load patterns.♻️ Suggested refactor
+ private static let deferredBonsplitGeometryFallbackDelay: TimeInterval = 0.02 + private func deferTerminalGeometryReconcileUntilBonsplitGeometryChange(reason: String) { layoutFollowUpDeferredBonsplitGeometryGeneration &+= 1 let generation = layoutFollowUpDeferredBonsplitGeometryGeneration @@ } layoutFollowUpDeferredBonsplitGeometryFallbackWorkItem = workItem - DispatchQueue.main.asyncAfter(deadline: .now() + 0.02, execute: workItem) + DispatchQueue.main.asyncAfter( + deadline: .now() + Self.deferredBonsplitGeometryFallbackDelay, + execute: workItem + ) }Based on learnings Repo manaflow-ai/cmux — Preference: keep lower-confidence geometry/focus/portal behavior changes out of stabilization checkpoints; evaluate them in a separate, narrowly scoped follow-up PR with DEBUG-gated toggles and diagnostics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 8517 - 8544, Replace the hardcoded 0.02 delay in deferTerminalGeometryReconcileUntilBonsplitGeometryChange with a named, tunable constant (e.g., layoutDeferredBonsplitGeometryFallbackDelay or kLayoutDeferredFallbackDelay) and use that constant in the DispatchQueue.main.asyncAfter call; make the constant configurable for DEBUG builds (conditional compilation or a debug-only override) so tests and local tuning can adjust timing, and update any references to layoutFollowUpDeferredBonsplitGeometryFallbackWorkItem, layoutFollowUpDeferredBonsplitGeometryGeneration, and scheduleTerminalGeometryReconcile to use the new constant for clarity and future calibration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Workspace.swift`:
- Around line 8517-8544: Replace the hardcoded 0.02 delay in
deferTerminalGeometryReconcileUntilBonsplitGeometryChange with a named, tunable
constant (e.g., layoutDeferredBonsplitGeometryFallbackDelay or
kLayoutDeferredFallbackDelay) and use that constant in the
DispatchQueue.main.asyncAfter call; make the constant configurable for DEBUG
builds (conditional compilation or a debug-only override) so tests and local
tuning can adjust timing, and update any references to
layoutFollowUpDeferredBonsplitGeometryFallbackWorkItem,
layoutFollowUpDeferredBonsplitGeometryGeneration, and
scheduleTerminalGeometryReconcile to use the new constant for clarity and future
calibration.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f3102f184
ℹ️ 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".
gaelic-ghost
marked this pull request as draft
March 19, 2026 00:50
Contributor
Author
|
@coderabbitai review |
@gaelic-ghost I have started the AI code review. It will take a few minutes to complete. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
4 issues found across 3 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/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:9441">
P2: Hardcoded numeric shortcut checks (Cmd/Ctrl + `Int(chars)`) bypass the configurable `KeyboardShortcutSettings` for selectWorkspaceByNumber/selectSurfaceByNumber and remove the previous normalization/keycode fallback. This can break user-customized modifiers and non‑US layouts where digit characters differ.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:3864">
P2: Cancellation no longer reaches the running SCP process. `uploadDroppedFilesLocked` now calls `scpExec` without passing the `TerminalImageTransferOperation`, and `runProcess` no longer checks or installs a cancellation handler. As a result, mid-transfer cancellations won’t terminate the active `scp` and only take effect between files, leaving long uploads running until timeout.</violation>
<violation number="2" location="Sources/Workspace.swift:4982">
P2: Branch-directory dedup canonicalization regressed for remote paths by expanding `~` without inferred remote home, causing duplicate/unstable sidebar entries.</violation>
<violation number="3" location="Sources/Workspace.swift:10147">
P2: Remote terminal identity is dropped during detach/attach, so moving a remote terminal tab decrements activeRemoteTerminalSessionCount in the source workspace but never increments it in the destination workspace. This leaves remote-session tracking inaccurate after cross-workspace moves.</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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 9155-9170: The branch that lets auxiliary non-main windows skip
the routing guard (inside the hasEventWindowContext &&
!didSynchronizeShortcutContext check) allows later shortcuts to run against the
wrong main-window context; instead, keep the routing guard intact and only
special-case auxiliary windows for the close-window shortcut. Concretely, in the
block that checks if let eventTargetWindow,
!isMainTerminalWindow(eventTargetWindow) do not fall through to the rest of the
method unconditionally — add a check for the specific close-window shortcut
(e.g., detect Command-W via the NSEvent or an existing close shortcut helper)
and only allow that path to continue for auxiliary windows; for all other
shortcuts return false so tabManager/main-window routing remains correct (update
or add a small helper like isCloseShortcut(event) if needed and reference
eventTargetWindow, isMainTerminalWindow(_:),
shortcutEventHasAddressableWindow(_:) and
synchronizeShortcutRoutingContext(event:) when making the change).
- Around line 9440-9463: The Cmd/Ctrl digit shortcut checks in AppDelegate.swift
reject cases where the keyboard layout requires Shift (e.g., AZERTY), because
they only match flags == [.command] or [.control]; update the handler that calls
WorkspaceShortcutMapper.workspaceIndex(...) and the subsequent control-flag
branch that calls tabManager.selectTab(at:), tabManager.selectSurface(at:), and
tabManager.selectLastSurface() to also accept the Shift modifier (e.g., flags ==
[.command, .shift] and flags == [.control, .shift]) or, preferably, decode the
keyCode using the same digit keyCode→digit mapping logic used elsewhere (lines
around the existing keyCode mapping) and use
WorkspaceShortcutMapper.workspaceIndex(forCommandDigit:) with that decoded digit
so shortcuts work across layouts. Ensure both Cmd+digit and Ctrl+digit flows use
the new detection.
In `@Sources/TerminalWindowPortal.swift`:
- Around line 593-599: The external geometry coalescing path updates
externalGeometryQueuedCoverage but fails to preserve the captured
requiresSettledLayout from earlier calls, causing a later settled-layout request
to be treated as intermediate; when coalescing (where
externalGeometryQueuedCoverage is merged and syncCoalescingEpoch used), also
merge/OR the requiresSettledLayout requirement into the queued state (e.g.,
add/update an externalGeometryQueuedRequiresSettledLayout boolean or fold it
into the existing queued flags) so the eventual execution that branches on
requiresSettledLayout sees the stronger (settled) requirement; update all
coalescing sites that touch
externalGeometryQueuedCoverage/hasExternalGeometrySyncScheduled/syncCoalescingEpoch
to maintain this flag.
- Around line 1372-1404: The primary sync result from
synchronizeHostedView(withId:) is currently discarded; capture its return value
(when primaryHostedId is non-nil), combine it with the result of
synchronizeAllHostedViews(excluding:) into a single aggregate result (e.g.,
treat the overall result as .transient if either result is .transient, and
.settled only if both are .settled), and then use that combined result instead
of syncResult for the DEBUG logging, the markFullSyncSatisfied() check, and the
decision to call scheduleDeferredFullSynchronizeAll(); this prevents transient
primary anchors from being treated as settled and avoids unnecessary deferred
full passes for single-pane syncs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f665fd0-56d1-4433-8831-90a9bd927db5
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/TerminalWindowPortal.swiftSources/Workspace.swift
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/Workspace.swift
Contributor
Author
|
@coderabbitai review |
@gaelic-ghost I have started the AI code review. It will take a few minutes to complete. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
1 issue found across 3 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/TerminalWindowPortal.swift">
<violation number="1" location="Sources/TerminalWindowPortal.swift:887">
P2: Full-sync is marked satisfied from the secondary pass even when the primary anchor sync may still be transient, which can suppress needed retry scheduling via `alreadySatisfied` checks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
gaelic-ghost
commented
Mar 23, 2026
Contributor
Author
|
@coderabbitai review |
@gaelic-ghost I have started the AI code review. It will take a few minutes to complete. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
gaelic-ghost
marked this pull request as ready for review
March 23, 2026 03:32
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62674f1e99
ℹ️ 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".
Comment on lines
+6898
to
6899
| guard let sourceSurface = terminalPanel.surface.surface else { continue } | ||
| var config = cmuxInheritedSurfaceConfig( |
There was a problem hiding this comment.
inheritedTerminalConfig now accepts any non-nil surface pointer, but TerminalSurface.hasLiveSurface explicitly documents that non-nil is insufficient before calling Ghostty C APIs. This path immediately calls cmuxInheritedSurfaceConfig (which dereferences the surface), so during portal teardown/reparent churn it can pass a closing surface and trigger crashes or invalid inherited settings.
Useful? React with 👍 / 👎.
Comment on lines
10449
to
10453
| let hasEventChars = !(eventCharsIgnoringModifiers?.isEmpty ?? true) | ||
| let eventCharsAreASCII = eventCharsIgnoringModifiers?.allSatisfy(\.isASCII) ?? true | ||
| if hasEventChars, | ||
| eventCharsAreASCII, | ||
| flags.contains(.command), | ||
| !flags.contains(.control), | ||
| shouldRequireCharacterMatchForCommandShortcut(shortcutKey: shortcutKey) { |
There was a problem hiding this comment.
The command-shortcut guard now returns false whenever charactersIgnoringModifiers is non-empty, even if it is non-ASCII IME text. In that case the layout/keycode fallback path is never reached, so Cmd+letter shortcuts (e.g., workspace and app actions) stop working while Korean/Japanese/Chinese input sources are active.
Useful? React with 👍 / 👎.
Comment on lines
+123
to
+124
| localized: "error.remoteDrop.uploadFailed", | ||
| defaultValue: "Failed to upload dropped file: \(detail)" |
There was a problem hiding this comment.
error.remoteDrop.uploadFailed is localized with a %@ placeholder, but this code now returns String(localized:) directly without formatting, so translated messages render a literal %@ and omit the actual error text. This regresses user-facing diagnostics for remote-drop failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Sources/TerminalWindowPortal.swift (1)
1372-1404: Primary anchor sync result is discarded; aggregate may be incomplete.Line 1373 discards the primary
synchronizeHostedView(withId:)result with_ =, while lines 1379-1403 only use the sibling sweep result. This means a transient primary pane could causemarkFullSyncSatisfied()to run if siblings settle, or single-pane anchor syncs will always have.vacuousresult.The
scheduleDeferredFullSynchronizeAll()at line 1404 provides a fallback, but this may cause unnecessary deferred syncs. This was flagged in past review comments about folding the primary result into the aggregate.♻️ Suggested fix: Aggregate primary and sibling results
let anchorId = ObjectIdentifier(anchorView) let primaryHostedId = hostedByAnchorId[anchorId] -if let primaryHostedId { - _ = synchronizeHostedView(withId: primaryHostedId) -} +let primaryResult: HostedSyncResult = { + guard let primaryHostedId else { return .skipped } + return synchronizeHostedView(withId: primaryHostedId) +}() // Failsafe: during aggressive divider drags/structural churn, one anchor can miss a // geometry callback while another fires. Reconcile all mapped hosted views so no stale // frame remains "stuck" onscreen until the next interaction. -let syncResult = synchronizeAllHostedViews(excluding: primaryHostedId) +let siblingResult = synchronizeAllHostedViews(excluding: primaryHostedId) +let syncResult: FullSyncPassResult = { + switch (primaryResult, siblingResult) { + case (.transient, _), (_, .transient): return .transient + case (.settled, _), (_, .settled): return .settled + default: return .vacuous + } +}()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalWindowPortal.swift` around lines 1372 - 1404, The primary call to synchronizeHostedView(withId:) currently discards its result, so combine its return value with the sibling sweep result from synchronizeAllHostedViews(excluding:) into an aggregate result (e.g., treat .transient if either is .transient, .settled only if both are .settled, .vacuous if both vacuous) and use that aggregate for the DEBUG UITest event logging and the markFullSyncSatisfied() decision; keep scheduleDeferredFullSynchronizeAll() as the fallback but only schedule it based on the aggregated outcome.Sources/Workspace.swift (1)
10313-10330: Avoid stacking the bespoke move refresh on top of the new coalescer.
didMoveTabnow runsscheduleMovedTerminalRefresh(...)and thenscheduleTerminalGeometryReconcile(...)for the same move. Since the helper already does two manual refresh passes, a normal move gets three geometry/refresh opportunities. I'd either gate the bespoke path behind an actual attach/geometry failure or move it to a follow-up once the unified scheduler is settled.Based on learnings, "Preference: keep lower-confidence geometry/focus/portal behavior changes out of stabilization checkpoints; evaluate them in a separate, narrowly scoped follow-up PR with DEBUG-gated toggles and diagnostics."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 10313 - 10330, didMoveTab is scheduling both scheduleMovedTerminalRefresh(...) and scheduleTerminalGeometryReconcile(...), causing redundant refreshes; change didMoveTab so the bespoke scheduleMovedTerminalRefresh(panelId: movedPanelId, reason: "workspace.didMoveTab") is not always run alongside scheduleTerminalGeometryReconcile(reason: "workspace.didMoveTab"). Either remove the unconditional call to scheduleMovedTerminalRefresh and rely on scheduleTerminalGeometryReconcile, or gate scheduleMovedTerminalRefresh behind a real-attach/geometry-failure check (use panelIdFromSurfaceId(tab.id) / movedPanelId comparison or a failure flag), or make it DEBUG-only with diagnostics, keeping normalizePinnedTabs(in:) and the final scheduleTerminalGeometryReconcile intact.
🤖 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/TerminalWindowPortal.swift`:
- Around line 593-599: scheduleExternalGeometrySynchronize currently computes
requiresSettledLayout once and coalescing updates only
externalGeometryQueuedCoverage, losing stronger settled-layout requirements from
later calls; add a stored Bool property
externalGeometryQueuedRequiresSettledLayout (like
deferredFullSyncQueuedForce/deferredFullSyncQueuedCoverage) and when coalescing
(updating externalGeometryQueuedCoverage in scheduleExternalGeometrySynchronize)
set externalGeometryQueuedRequiresSettledLayout =
externalGeometryQueuedRequiresSettledLayout || requiresSettledLayout so any
later request that requires settled layout is preserved, and use
externalGeometryQueuedRequiresSettledLayout (not the original local
requiresSettledLayout) when deciding to enqueue the extra queue-hop or force
settled-layout behavior in the rest of the synchronization flow.
In `@Sources/Workspace.swift`:
- Around line 3865-3867: The code currently only checks
normalizedLocalURL.isFileURL which is true for directories; update the
validation to ensure the URL points to a regular file (not a directory or
bundle) before spawning scp: query resource values (e.g. using
normalizedLocalURL.resourceValues(forKeys: [.isRegularFileKey, .isDirectoryKey])
or FileManager APIs) and if the URL is not a regular file (or is a directory)
throw RemoteDropUploadError.invalidFileURL (or add a more specific
RemoteDropUploadError case if desired). Ensure this check is performed where
normalizedLocalURL is defined so directories are rejected up front rather than
handed off to the scp subprocess.
- Around line 5636-5647: The closure set in configureTerminalPanel captures the
current Workspace via terminalPanel.onRequestWorkspacePaneFlash, so when a
TerminalPanel is moved by attachDetachedSurface (which only updates workspaceId)
you must rebind that callback; after calling terminalPanel.updateWorkspaceId(id)
in attachDetachedSurface (or wherever you move the panel) call
configureTerminalPanel(terminalPanel) so the terminal’s
onRequestWorkspacePaneFlash closure references the new workspace and sends
pane-flash requests to the correct workspace (use the TerminalPanel type and the
configureTerminalPanel, attachDetachedSurface, updateWorkspaceId,
onRequestWorkspacePaneFlash symbols to locate and apply the change).
- Around line 8313-8315: The change in triggerFocusFlash(panelId: UUID) should
not call panels[panelId]?.triggerFlash() directly because that bypasses
WorkspaceAttentionCoordinator.decideFlash(...) and its suppression rules; update
triggerFocusFlash to route the flash request through the attention coordinator
(call the coordinator's decideFlash / decideFlash(for:) API with the panelId or
panel instance instead of calling Panel.triggerFlash() directly), ensuring the
coordinator enforces existing suppression when another panel already has an
unread/manual indicator.
---
Nitpick comments:
In `@Sources/TerminalWindowPortal.swift`:
- Around line 1372-1404: The primary call to synchronizeHostedView(withId:)
currently discards its result, so combine its return value with the sibling
sweep result from synchronizeAllHostedViews(excluding:) into an aggregate result
(e.g., treat .transient if either is .transient, .settled only if both are
.settled, .vacuous if both vacuous) and use that aggregate for the DEBUG UITest
event logging and the markFullSyncSatisfied() decision; keep
scheduleDeferredFullSynchronizeAll() as the fallback but only schedule it based
on the aggregated outcome.
In `@Sources/Workspace.swift`:
- Around line 10313-10330: didMoveTab is scheduling both
scheduleMovedTerminalRefresh(...) and scheduleTerminalGeometryReconcile(...),
causing redundant refreshes; change didMoveTab so the bespoke
scheduleMovedTerminalRefresh(panelId: movedPanelId, reason:
"workspace.didMoveTab") is not always run alongside
scheduleTerminalGeometryReconcile(reason: "workspace.didMoveTab"). Either remove
the unconditional call to scheduleMovedTerminalRefresh and rely on
scheduleTerminalGeometryReconcile, or gate scheduleMovedTerminalRefresh behind a
real-attach/geometry-failure check (use panelIdFromSurfaceId(tab.id) /
movedPanelId comparison or a failure flag), or make it DEBUG-only with
diagnostics, keeping normalizePinnedTabs(in:) and the final
scheduleTerminalGeometryReconcile intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1852eba-a55e-4814-a65a-63ad29a646d5
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/TerminalWindowPortal.swiftSources/Workspace.swift
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/AppDelegate.swift
Comment on lines
+3865
to
+3867
| let normalizedLocalURL = localURL.standardizedFileURL | ||
| guard normalizedLocalURL.isFileURL else { | ||
| throw RemoteDropUploadError.invalidFileURL |
There was a problem hiding this comment.
Reject directories before spawning scp.
URL.isFileURL is also true for folders and bundle packages. This path invokes scp without -r, so directory drops make it to the subprocess and fail as a generic upload error instead of being rejected up front. An isRegularFile/isDirectory check here would give users a clear validation failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 3865 - 3867, The code currently only
checks normalizedLocalURL.isFileURL which is true for directories; update the
validation to ensure the URL points to a regular file (not a directory or
bundle) before spawning scp: query resource values (e.g. using
normalizedLocalURL.resourceValues(forKeys: [.isRegularFileKey, .isDirectoryKey])
or FileManager APIs) and if the URL is not a regular file (or is a directory)
throw RemoteDropUploadError.invalidFileURL (or add a more specific
RemoteDropUploadError case if desired). Ensure this check is performed where
normalizedLocalURL is defined so directories are rejected up front rather than
handed off to the scp subprocess.
Comment on lines
+5636
to
+5647
| private func configureTerminalPanel(_ terminalPanel: TerminalPanel) { | ||
| terminalPanel.onRequestWorkspacePaneFlash = { [weak self, weak terminalPanel] reason in | ||
| guard let self, let terminalPanel else { return } | ||
| self.triggerWorkspacePaneFlash(panelId: terminalPanel.id, reason: reason) | ||
| } | ||
| } | ||
|
|
||
| private func triggerWorkspacePaneFlash(panelId: UUID, reason: WorkspaceAttentionFlashReason) { | ||
| tmuxWorkspaceFlashPanelId = panelId | ||
| tmuxWorkspaceFlashReason = reason | ||
| tmuxWorkspaceFlashToken &+= 1 | ||
| } |
There was a problem hiding this comment.
Rebind this callback after moving a terminal to another workspace.
This closure closes over the current Workspace. attachDetachedSurface(...) only updates workspaceId, so a moved terminal keeps sending pane-flash requests to the source workspace — or drops them entirely if that workspace is gone — until the callback is reinstalled.
Suggested follow-up
if let terminalPanel = detached.panel as? TerminalPanel {
terminalPanel.updateWorkspaceId(id)
configureTerminalPanel(terminalPanel)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 5636 - 5647, The closure set in
configureTerminalPanel captures the current Workspace via
terminalPanel.onRequestWorkspacePaneFlash, so when a TerminalPanel is moved by
attachDetachedSurface (which only updates workspaceId) you must rebind that
callback; after calling terminalPanel.updateWorkspaceId(id) in
attachDetachedSurface (or wherever you move the panel) call
configureTerminalPanel(terminalPanel) so the terminal’s
onRequestWorkspacePaneFlash closure references the new workspace and sends
pane-flash requests to the correct workspace (use the TerminalPanel type and the
configureTerminalPanel, attachDetachedSurface, updateWorkspaceId,
onRequestWorkspacePaneFlash symbols to locate and apply the change).
Comment on lines
8313
to
8315
| func triggerFocusFlash(panelId: UUID) { | ||
| requestAttentionFlash(panelId: panelId, reason: .navigation) | ||
| panels[panelId]?.triggerFlash() | ||
| } |
There was a problem hiding this comment.
Keep navigation flashes behind the attention coordinator.
triggerFocusFlash now calls triggerFlash() directly, which bypasses WorkspaceAttentionCoordinator.decideFlash(...) in Sources/Panels/Panel.swift. That drops the existing suppression rule when another panel already has an unread/manual indicator, so focus-navigation can become noisy again.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 8313 - 8315, The change in
triggerFocusFlash(panelId: UUID) should not call panels[panelId]?.triggerFlash()
directly because that bypasses WorkspaceAttentionCoordinator.decideFlash(...)
and its suppression rules; update triggerFocusFlash to route the flash request
through the attention coordinator (call the coordinator's decideFlash /
decideFlash(for:) API with the panelId or panel instance instead of calling
Panel.triggerFlash() directly), ensuring the coordinator enforces existing
suppression when another panel already has an unread/manual indicator.
Contributor
Greptile SummaryThis PR fixes workspace-remount geometry churn and pane-resize flicker by coalescing portal sync requests and deferring terminal geometry reconciliation until Bonsplit reports its final layout. It also restores several mainline hooks (workspace shortcuts, remote-transfer fixes, auxiliary-window close routing) needed for the branch to build against current Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Bonsplit topology change\ne.g. didClosePane, didSplit] --> B{preferBonsplitGeometryCallback?}
B -- yes --> C[deferTerminalGeometryReconcile\ninstall 20ms fallback timer]
B -- no --> D[scheduleTerminalGeometryReconcile\nimmediate]
C --> E[splitTabBar didChangeGeometry\nfired by Bonsplit]
E --> F[consumeDeferredBonsplitGeometryReason\ncancel fallback timer]
F --> D
C -. 20ms timeout .-> D
D --> G[beginEventDrivenLayoutFollowUp]
G --> H{alreadyActive cycle?}
H -- yes --> I[merge into active cycle\nscheduleLayoutFollowUpAttempt]
H -- no --> J[attemptEventDrivenLayoutFollowUp]
I --> J
J --> K[reconcileTerminalGeometryPass]
K --> L{transientPortalRemount?}
L -- yes --> M[defer: requestViewReattach\nneedsFollowUp=true]
L -- no --> N[reconcileGeometryNow]
N --> O{geometry changed?}
O -- yes --> P[forceRefresh + markFullSyncSatisfied]
O -- no --> Q[skip no-op portal update\nreturn .settled]
M --> R[scheduleLayoutFollowUpAttempt\nback-off retry]
P --> S{needsMoreWork?}
Q --> S
S -- yes --> R
S -- no --> T[clearLayoutFollowUp]
|
Comment on lines
5602
to
+5608
| ) | ||
| #endif | ||
| guard let context = preferredMainWindowContextForWorkspaceCreation(event: event, debugSource: debugSource) else { | ||
| let orphanedContexts = Array( | ||
| Set( | ||
| mainWindowContexts.values.compactMap { context -> ObjectIdentifier? in | ||
| resolvedWindow(for: context) == nil ? ObjectIdentifier(context) : nil |
Contributor
There was a problem hiding this comment.
The orphaned-context sweep is placed directly in the guard let context = preferredMainWindowContextForWorkspaceCreation(...) else branch, so it runs every time no context is found — not just during genuine orphan recovery. If preferredMainWindowContextForWorkspaceCreation legitimately fails because all windows are busy/minimised, this will still try to scan and discard orphans on every event that triggers workspace creation. That's likely harmless in practice, but the intent appears to be recovery from rare stale-context leaks, so it might be worth gating this on a debug flag or at least ensuring it is not expensive on the hot path.
Additionally, the double-pass to collect orphan ObjectIdentifiers (first into a Set, then a compactMap to recover the context objects) is unnecessarily complex — the identifiers don't deduplicate anything useful here because the first compactMap already emits one-per-context:
let orphanedContexts = mainWindowContexts.values.filter { context in
resolvedWindow(for: context) == nil
}
for orphanedContext in orphanedContexts {
discardOrphanedMainWindowContext(orphanedContext)
}| @@ -8886,11 +8843,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent | |||
| private func handleCustomShortcut(event: NSEvent) -> Bool { | |||
Contributor
There was a problem hiding this comment.
The observer is now registered on UserDefaults.didChangeNotification instead of KeyboardShortcutSettings.didChangeNotification. UserDefaults.didChangeNotification fires for any UserDefaults write — including writes completely unrelated to keyboard shortcuts (e.g. scroll position, window state, feature flags). This will cause shortcutSettingsDidChange() to be called far more often than intended, potentially causing unnecessary shortcut re-registration churn on every unrelated preferences write.
Comment on lines
8313
to
+8335
| func triggerFocusFlash(panelId: UUID) { | ||
| requestAttentionFlash(panelId: panelId, reason: .navigation) | ||
| panels[panelId]?.triggerFlash() | ||
| } | ||
|
|
||
| func triggerNotificationFocusFlash( | ||
| panelId: UUID, | ||
| requiresSplit: Bool = false, | ||
| shouldFocus: Bool = true | ||
| ) { | ||
| guard terminalPanel(for: panelId) != nil else { return } | ||
| guard let terminalPanel = terminalPanel(for: panelId) else { return } | ||
| if shouldFocus { | ||
| focusPanel(panelId) | ||
| } | ||
| let isSplit = bonsplitController.allPaneIds.count > 1 || panels.count > 1 | ||
| if requiresSplit && !isSplit { | ||
| return | ||
| } | ||
| requestAttentionFlash(panelId: panelId, reason: .notificationArrival) | ||
| terminalPanel.triggerFlash() | ||
| } | ||
|
|
||
| func triggerNotificationDismissFlash(panelId: UUID) { | ||
| guard terminalPanel(for: panelId) != nil else { return } | ||
| requestAttentionFlash(panelId: panelId, reason: .notificationDismiss) | ||
| guard let terminalPanel = terminalPanel(for: panelId) else { return } | ||
| terminalPanel.triggerFlash() |
Contributor
There was a problem hiding this comment.
The previous code routed all three flash entry points through requestAttentionFlash, which called WorkspaceAttentionCoordinator.decideFlash and returned early when decision.isAllowed == false. That gate-keeping logic consulted the notification store's unread state, focused-read indicator, and manual-unread sets to suppress redundant or inappropriate flashes.
All three callers now call triggerFlash() unconditionally:
triggerFocusFlash(navigation flash)triggerNotificationFocusFlash(arrival flash)triggerNotificationDismissFlash(dismiss flash)
If the coordinator was suppressing flashes in cases like "already-focused panel receiving a notification" or "dismiss flash on an already-read panel", those suppression paths are now gone, and users may see spurious visual flashes that were previously intentionally blocked. If this removal is intentional for mainline simplification, it should be documented (or the coordinator logic should be folded into TerminalPanel.triggerFlash).
Comment on lines
9679
to
+9680
| alert.messageText = String(localized: "dialog.closeTab.title", defaultValue: "Close tab?") | ||
|
|
||
| let panelName: String? = { | ||
| guard let panelId = panelIdFromSurfaceId(tabId) else { return nil } | ||
| if let custom = panelCustomTitles[panelId], !custom.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | ||
| return custom | ||
| } | ||
| if let title = panelTitles[panelId], !title.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | ||
| return title | ||
| } | ||
| if let dir = panelDirectories[panelId], !dir.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | ||
| return (dir as NSString).lastPathComponent | ||
| } | ||
| return nil | ||
| }() | ||
|
|
||
| if let panelName { | ||
| alert.informativeText = String(localized: "dialog.closeTab.messageNamed", defaultValue: "This will close \"\(panelName)\".") | ||
| } else { | ||
| alert.informativeText = String(localized: "dialog.closeTab.message", defaultValue: "This will close the current tab.") | ||
| } | ||
| alert.informativeText = String(localized: "dialog.closeTab.message", defaultValue: "This will close the current tab.") |
Contributor
There was a problem hiding this comment.
The block that resolved the panel's custom title, display title, or directory name for the confirmation dialog was removed. The dialog now always shows the generic "This will close the current tab." regardless of whether the tab has a known name.
Users who have named their panes (or panes with an active directory shown as the title) can no longer confirm which tab they're about to close — they see only the generic fallback. This is a minor but noticeable UX regression for multi-pane sessions. If the panel-name resolution logic was intentionally dropped as part of the mainline restore, it is worth a follow-up issue to restore the context.
| @MainActor | ||
| final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCenterDelegate, NSMenuItemValidation { | ||
| nonisolated(unsafe) static var shared: AppDelegate? | ||
| static var shared: AppDelegate? |
Contributor
There was a problem hiding this comment.
nonisolated(unsafe) removal creates concurrency ambiguity with new #if DEBUG callers
AppDelegate is @MainActor-isolated, so AppDelegate.shared is now a @MainActor-isolated property. WindowTerminalPortal (an NSObject subclass, not actor-isolated) calls AppDelegate.shared?.recordUITestTerminalGeometryEvent(...) from inside DispatchQueue.main.async { } closures at multiple new #if DEBUG sites in TerminalWindowPortal.swift (lines ~878, ~1391, ~1500, ~2235).
DispatchQueue.main.async is not equivalent to a @MainActor context from the compiler's perspective. With Swift strict concurrency (SWIFT_STRICT_CONCURRENCY = complete), these would be actor-isolation errors. With targeted they are likely warnings today but errors under Swift 6.
The original nonisolated(unsafe) annotation existed precisely to allow cross-context access while signalling that the caller was responsible for synchronisation. Since all of these new callers are already on the main thread at runtime, the fix could be either:
- Restore
nonisolated(unsafe)if cross-context access is expected, or - Annotate
WindowTerminalPortal(or the relevant closures) as@MainActorto satisfy the compiler.
gaelic-ghost
marked this pull request as draft
March 23, 2026 03:45
There was a problem hiding this comment.
1 issue found across 3 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/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:6800">
P1: Removed `hasLiveSurface` guard allows stale Ghostty surface pointers to reach `cmuxCurrentSurfaceFontSizePoints`, which dereferences an unretained CTFont pointer; docs note non‑nil surfaces may already be closing, so this can crash.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| lastTerminalConfigInheritancePanelId = terminalPanel.id | ||
| if terminalPanel.surface.hasLiveSurface, | ||
| let sourceSurface = terminalPanel.surface.surface, | ||
| if let sourceSurface = terminalPanel.surface.surface, |
There was a problem hiding this comment.
P1: Removed hasLiveSurface guard allows stale Ghostty surface pointers to reach cmuxCurrentSurfaceFontSizePoints, which dereferences an unretained CTFont pointer; docs note non‑nil surfaces may already be closing, so this can crash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Workspace.swift, line 6800:
<comment>Removed `hasLiveSurface` guard allows stale Ghostty surface pointers to reach `cmuxCurrentSurfaceFontSizePoints`, which dereferences an unretained CTFont pointer; docs note non‑nil surfaces may already be closing, so this can crash.</comment>
<file context>
@@ -6852,8 +6797,7 @@ final class Workspace: Identifiable, ObservableObject {
lastTerminalConfigInheritancePanelId = terminalPanel.id
- if terminalPanel.surface.hasLiveSurface,
- let sourceSurface = terminalPanel.surface.surface,
+ if let sourceSurface = terminalPanel.surface.surface,
let runtimePoints = cmuxCurrentSurfaceFontSizePoints(sourceSurface) {
let existing = terminalInheritanceFontPointsByPanelId[terminalPanel.id]
</file context>
Suggested change
| if let sourceSurface = terminalPanel.surface.surface, | |
| if terminalPanel.surface.hasLiveSurface, | |
| let sourceSurface = terminalPanel.surface.surface, |
There was a problem hiding this comment.
2 issues found across 3 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/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:124">
P2: `error.remoteDrop.uploadFailed` now bypasses format substitution while translations still contain `%@`, causing broken/placeholder output in localized locales.</violation>
<violation number="2" location="Sources/Workspace.swift:6800">
P1: Removed live-surface/stale-pointer protections allow `ghostty_surface_quicklook_font` results to be dereferenced from possibly stale Ghostty pointers, reintroducing crash risk.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| lastTerminalConfigInheritancePanelId = terminalPanel.id | ||
| if terminalPanel.surface.hasLiveSurface, | ||
| let sourceSurface = terminalPanel.surface.surface, | ||
| if let sourceSurface = terminalPanel.surface.surface, |
There was a problem hiding this comment.
P1: Removed live-surface/stale-pointer protections allow ghostty_surface_quicklook_font results to be dereferenced from possibly stale Ghostty pointers, reintroducing crash risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Workspace.swift, line 6800:
<comment>Removed live-surface/stale-pointer protections allow `ghostty_surface_quicklook_font` results to be dereferenced from possibly stale Ghostty pointers, reintroducing crash risk.</comment>
<file context>
@@ -6852,8 +6797,7 @@ final class Workspace: Identifiable, ObservableObject {
lastTerminalConfigInheritancePanelId = terminalPanel.id
- if terminalPanel.surface.hasLiveSurface,
- let sourceSurface = terminalPanel.surface.surface,
+ if let sourceSurface = terminalPanel.surface.surface,
let runtimePoints = cmuxCurrentSurfaceFontSizePoints(sourceSurface) {
let existing = terminalInheritanceFontPointsByPanelId[terminalPanel.id]
</file context>
Suggested change
| if let sourceSurface = terminalPanel.surface.surface, | |
| if terminalPanel.surface.hasLiveSurface, | |
| let sourceSurface = terminalPanel.surface.surface, |
| ), | ||
| detail | ||
| localized: "error.remoteDrop.uploadFailed", | ||
| defaultValue: "Failed to upload dropped file: \(detail)" |
There was a problem hiding this comment.
P2: error.remoteDrop.uploadFailed now bypasses format substitution while translations still contain %@, causing broken/placeholder output in localized locales.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Workspace.swift, line 124:
<comment>`error.remoteDrop.uploadFailed` now bypasses format substitution while translations still contain `%@`, causing broken/placeholder output in localized locales.</comment>
<file context>
@@ -127,22 +115,13 @@ private enum RemoteDropUploadError: LocalizedError {
- ),
- detail
+ localized: "error.remoteDrop.uploadFailed",
+ defaultValue: "Failed to upload dropped file: \(detail)"
)
}
</file context>
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.
Closes #1682
Closes #1681
What changed
mainTesting
./scripts/reload.sh --tag workspace-geometryNotes
Checklist
Summary by cubic
Fixes wrong intermediate resizes when switching workspaces and cuts pane flicker by coalescing geometry updates and tightening portal sync. Restores mainline workspace hooks plus shortcut/remote transfer fixes and corrects auxiliary shortcut routing, closing #1682 and materially reducing resize churn in #1681.
Bug Fixes
Bonsplitreports the final layout (≈20ms fallback), merges requests into the active follow‑up cycle, and pauses transient portal remounts until the host window returns.Refactors
Written for commit 62674f1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Removed