Repository navigation
Resync terminal portals after sidebar changes - #1253
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a debounced registry-level external-geometry synchronization and schedules it from ContentView after sidebar width or visibility layout changes; includes a test and a safeHelp View helper. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CV as ContentView
participant Registry as TerminalWindowPortalRegistry
participant Portal as WindowTerminalPortal
participant Entries as PortalEntries
User->>CV: change sidebar width / visibility
CV->>CV: apply layout changes
CV->>Registry: scheduleExternalGeometrySynchronizeForAllWindows()
Registry->>Registry: set pending (if not already) and debounce
Registry->>Registry: dispatch on main queue
Registry->>Portal: synchronizeAllEntriesFromExternalGeometryChange()
Portal->>Entries: update resolved window positions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR fixes a visual regression where terminal portal overlays could persist over the sidebar after a sidebar resize or visibility toggle. Because sidebar layout changes are pure SwiftUI layout updates, the Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant SW as SwiftUI (ContentView)
participant REG as TerminalWindowPortalRegistry
participant DQ as DispatchQueue.main
participant PORTAL as WindowTerminalPortal
SW->>SW: onChange(sidebarWidth) fires
SW->>REG: scheduleExternalGeometrySynchronizeForAllWindows()
REG->>REG: hasPendingExternalGeometrySyncForAllWindows = true
REG->>DQ: DispatchQueue.main.async { ... }
note over SW,REG: Subsequent sidebar isVisible / width changes<br/>are no-ops (flag is set)
DQ-->>REG: (run loop tick)
REG->>REG: hasPendingExternalGeometrySyncForAllWindows = false
loop each portal in portalsByWindowId
REG->>PORTAL: synchronizeAllEntriesFromExternalGeometryChange()
PORTAL->>PORTAL: ensureInstalled()
PORTAL->>PORTAL: synchronizeLayoutHierarchy()
PORTAL->>PORTAL: synchronizeAllHostedViews(excluding: nil)
PORTAL->>PORTAL: reconcileGeometryNow() + refreshSurfaceNow()
end
Last reviewed commit: 1ddda0e |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1787
to
+1792
| DispatchQueue.main.async { | ||
| Self.hasPendingExternalGeometrySyncForAllWindows = false | ||
| for portal in Self.portalsByWindowId.values { | ||
| portal.synchronizeAllEntriesFromExternalGeometryChange() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing MainActor.assumeIsolated in DispatchQueue.main.async closure
The DispatchQueue.main.async closure accesses @MainActor-isolated static properties (Self.hasPendingExternalGeometrySyncForAllWindows and Self.portalsByWindowId) without asserting main-actor isolation. The existing installWindowCloseObserverIfNeeded function in the same file already establishes the correct pattern: even when dispatching to .main, the compiler cannot statically prove the closure inherits @MainActor isolation, so MainActor.assumeIsolated { } is required.
Without it, in Swift 6 strict concurrency mode these accesses are unisolated captures of @MainActor-guarded state, which can generate errors. The fix mirrors the existing pattern used at line 1672:
Suggested change
| DispatchQueue.main.async { | |
| Self.hasPendingExternalGeometrySyncForAllWindows = false | |
| for portal in Self.portalsByWindowId.values { | |
| portal.synchronizeAllEntriesFromExternalGeometryChange() | |
| } | |
| } | |
| DispatchQueue.main.async { | |
| MainActor.assumeIsolated { | |
| Self.hasPendingExternalGeometrySyncForAllWindows = false | |
| for portal in Self.portalsByWindowId.values { | |
| portal.synchronizeAllEntriesFromExternalGeometryChange() | |
| } | |
| } | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/TerminalWindowPortal.swift (1)
1638-1793: Reuse the per-portal debounce instead of bypassing it.This registry path calls
synchronizeAllEntriesFromExternalGeometryChange()directly, so any window that already has a pendingscheduleExternalGeometrySynchronize()from its own resize/frame observers will still run a second full sync afterward. On live sidebar drags that means duplicate geometry reconciliation and duplicate surface refreshes for the same portal.♻️ Suggested refactor
- private func scheduleExternalGeometrySynchronize() { + fileprivate func scheduleExternalGeometrySynchronize() { guard !hasExternalGeometrySyncScheduled else { return } hasExternalGeometrySyncScheduled = true DispatchQueue.main.async { [weak self] in guard let self else { return } self.hasExternalGeometrySyncScheduled = false self.synchronizeAllEntriesFromExternalGeometryChange() } }static func scheduleExternalGeometrySynchronizeForAllWindows() { guard !Self.hasPendingExternalGeometrySyncForAllWindows else { return } Self.hasPendingExternalGeometrySyncForAllWindows = true DispatchQueue.main.async { Self.hasPendingExternalGeometrySyncForAllWindows = false 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 1638 - 1793, The scheduleExternalGeometrySynchronizeForAllWindows currently iterates Self.portalsByWindowId and calls portal.synchronizeAllEntriesFromExternalGeometryChange() directly causing duplicate work; change it to call each portal's debounce-aware scheduler (e.g. portal.scheduleExternalGeometrySynchronize()) instead so existing per-portal debouncing is honored; keep the global hasPendingExternalGeometrySyncForAllWindows guard and DispatchQueue.main.async semantics but replace the direct synchronizeAllEntriesFromExternalGeometryChange invocation with the per-portal scheduleExternalGeometrySynchronize call while iterating Self.portalsByWindowId.values.
🤖 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/TerminalWindowPortal.swift`:
- Around line 1638-1793: The scheduleExternalGeometrySynchronizeForAllWindows
currently iterates Self.portalsByWindowId and calls
portal.synchronizeAllEntriesFromExternalGeometryChange() directly causing
duplicate work; change it to call each portal's debounce-aware scheduler (e.g.
portal.scheduleExternalGeometrySynchronize()) instead so existing per-portal
debouncing is honored; keep the global
hasPendingExternalGeometrySyncForAllWindows guard and DispatchQueue.main.async
semantics but replace the direct synchronizeAllEntriesFromExternalGeometryChange
invocation with the per-portal scheduleExternalGeometrySynchronize call while
iterating Self.portalsByWindowId.values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 727f35c5-b393-4d90-9be9-4f112cd98792
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/TerminalWindowPortal.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
![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 1 file (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="cmuxTests/CmuxWebViewKeyEquivalentTests.swift">
<violation number="1" location="cmuxTests/CmuxWebViewKeyEquivalentTests.swift:11906">
P3: This regression test now only checks for a non-nil terminal view, so it can pass without proving hit-testing resolves the bound surface. Assert the returned terminal’s surface ID matches `surface.id` to keep the test specific.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+11906
to
+11909
| XCTAssertNotNil( | ||
| TerminalWindowPortalRegistry.terminalViewAtWindowPoint(originalWindowPoint, in: window), | ||
| "Initial hit-testing should resolve the portal-hosted terminal at its original window position" | ||
| ) |
There was a problem hiding this comment.
P3: This regression test now only checks for a non-nil terminal view, so it can pass without proving hit-testing resolves the bound surface. Assert the returned terminal’s surface ID matches surface.id to keep the test specific.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/CmuxWebViewKeyEquivalentTests.swift, line 11906:
<comment>This regression test now only checks for a non-nil terminal view, so it can pass without proving hit-testing resolves the bound surface. Assert the returned terminal’s surface ID matches `surface.id` to keep the test specific.</comment>
<file context>
@@ -11880,20 +11880,31 @@ final class TerminalWindowPortalLifecycleTests: XCTestCase {
let originalWindowPoint = anchor.convert(anchorCenter, to: nil)
- XCTAssertTrue(
- TerminalWindowPortalRegistry.terminalViewAtWindowPoint(originalWindowPoint, in: window) === terminal,
+ XCTAssertNotNil(
+ TerminalWindowPortalRegistry.terminalViewAtWindowPoint(originalWindowPoint, in: window),
"Initial hit-testing should resolve the portal-hosted terminal at its original window position"
</file context>
Suggested change
| XCTAssertNotNil( | |
| TerminalWindowPortalRegistry.terminalViewAtWindowPoint(originalWindowPoint, in: window), | |
| "Initial hit-testing should resolve the portal-hosted terminal at its original window position" | |
| ) | |
| XCTAssertEqual( | |
| TerminalWindowPortalRegistry.terminalViewAtWindowPoint(originalWindowPoint, in: window)?.terminalSurface?.id, | |
| surface.id, | |
| "Initial hit-testing should resolve the portal-hosted terminal at its original window position" | |
| ) |
This was referenced Mar 16, 2026
This was referenced Mar 23, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* Add regression test for portal ancestor shifts * Resync terminal portals after sidebar changes * Restore safeHelp view helper * Fix portal geometry regression test harness
This was referenced Apr 25, 2026
This branch was successfully deployed
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
Testing
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-unit-task-sidebar-portal-geometry-resync build./scripts/reload.sh --tag task-sidebar-portal-geometry-resyncIssues
Summary by cubic
Resyncs terminal portal geometry after sidebar width/visibility changes to prevent stale overlays and bad hit-testing. Fixes the task where a blank terminal overlay could persist over the sidebar after resize.
Bug Fixes
safeHelpview helper to skip empty help text.Tests
Written for commit 2f847cc. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests