Repository navigation
Fix workspace layout follow-up spin loop - #1633
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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.
📝 WalkthroughWalkthroughThe changes introduce exponential backoff scheduling and stall detection to prevent infinite re-enqueueing loops in layout follow-up observers. A debug property exposing portal active state is added to GhosttyTerminalView. Changes
Possibly Related PRs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?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.
🧹 Nitpick comments (2)
Sources/Workspace.swift (2)
8152-8157: Consider using reconcile change flags indidMakeProgress.You now return
Boolfrom both reconcile methods, butdidMakeProgressonly checks pending-state transitions. Including these change flags would avoid classifying intermediate reconciliation as a stall when pending predicates remain true.♻️ Proposed refinement
- reconcileTerminalPortalVisibilityForCurrentRenderedLayout() + let terminalPortalDidChange = reconcileTerminalPortalVisibilityForCurrentRenderedLayout() let terminalPortalPending = terminalPortalVisibilityNeedsFollowUp() @@ - reconcileBrowserPortalVisibilityForCurrentRenderedLayout(reason: reason) + let browserPortalDidChange = reconcileBrowserPortalVisibilityForCurrentRenderedLayout(reason: reason) let browserVisibilityPending = browserPortalVisibilityNeedsFollowUp() @@ let didMakeProgress = (geometryPendingBefore && !layoutFollowUpNeedsGeometryPass) || (terminalPortalPendingBefore && !terminalPortalPending) || (browserVisibilityPendingBefore && !browserVisibilityPending) || (terminalFocusPendingBefore && !terminalFocusPending) || (browserPanelPendingBefore && !browserPanelPending) || - (browserExitPendingBefore && !browserExitPending) + (browserExitPendingBefore && !browserExitPending) || + terminalPortalDidChange || + browserPortalDidChangeAlso applies to: 8211-8217, 8297-8321, 8343-8402
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 8152 - 8157, didMakeProgress currently only checks pending-state predicates (terminalPortalVisibilityNeedsFollowUp, browserPortalVisibilityNeedsFollowUp) and therefore treats intermediate reconciliation as a stall even though the reconcile methods now return Bool change flags; update didMakeProgress to also consider the Bool results from reconcileTerminalPortalVisibilityForCurrentRenderedLayout() and reconcileBrowserPortalVisibilityForCurrentRenderedLayout() (and analogous reconcile calls at the other sites mentioned) so that any returned true from these methods counts as progress even if the "needsFollowUp" predicates remain true—call the reconcile methods, capture their Bool return values (e.g., terminalChanged, browserChanged), and include those flags in the progress determination along with the existing pending-state checks.
8035-8049: Make scheduled follow-up attempts cancellable across follow-up generations.
DispatchQueue.main.asyncAfterat Line 8057 is not tied to a cancellable work item today, so a stale callback can fire afterclearLayoutFollowUp()and bleed into a new cycle. It’s safer to store/cancel aDispatchWorkItem.♻️ Proposed refactor
@@ private var layoutFollowUpAttemptScheduled = false private var layoutFollowUpStalledAttemptCount = 0 + private var layoutFollowUpAttemptWorkItem: DispatchWorkItem? @@ private func clearLayoutFollowUp() { @@ + layoutFollowUpAttemptWorkItem?.cancel() + layoutFollowUpAttemptWorkItem = nil layoutFollowUpAttemptScheduled = false layoutFollowUpStalledAttemptCount = 0 } @@ private func scheduleLayoutFollowUpAttempt() { guard layoutFollowUpTimeoutWorkItem != nil else { return } guard !layoutFollowUpAttemptScheduled else { return } layoutFollowUpAttemptScheduled = true let delay = layoutFollowUpBackoffDelay() - DispatchQueue.main.asyncAfter(deadline: .now() + delay) { [weak self] in + layoutFollowUpAttemptWorkItem?.cancel() + let workItem = DispatchWorkItem { [weak self] in guard let self else { return } self.layoutFollowUpAttemptScheduled = false + self.layoutFollowUpAttemptWorkItem = nil self.attemptEventDrivenLayoutFollowUp() } + layoutFollowUpAttemptWorkItem = workItem + DispatchQueue.main.asyncAfter(deadline: .now() + delay, execute: workItem) }Also applies to: 8051-8061
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 8035 - 8049, The scheduled follow-up callbacks use DispatchQueue.main.asyncAfter without a cancellable work item, so create and use a DispatchWorkItem for these timeouts: allocate a DispatchWorkItem, assign it to the existing layoutFollowUpTimeoutWorkItem (or a new clearly named property if another asyncAfter is used), cancel any previous work item before assigning, schedule it with DispatchQueue.main.asyncAfter(deadline: .now() + delay, execute: workItem), and ensure clearLayoutFollowUp continues to cancel and nil out layoutFollowUpTimeoutWorkItem; apply the same pattern to the other asyncAfter usages referenced (the follow-up attempt scheduling range) so all scheduled callbacks are cancellable across follow-up generations.
🤖 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 8152-8157: didMakeProgress currently only checks pending-state
predicates (terminalPortalVisibilityNeedsFollowUp,
browserPortalVisibilityNeedsFollowUp) and therefore treats intermediate
reconciliation as a stall even though the reconcile methods now return Bool
change flags; update didMakeProgress to also consider the Bool results from
reconcileTerminalPortalVisibilityForCurrentRenderedLayout() and
reconcileBrowserPortalVisibilityForCurrentRenderedLayout() (and analogous
reconcile calls at the other sites mentioned) so that any returned true from
these methods counts as progress even if the "needsFollowUp" predicates remain
true—call the reconcile methods, capture their Bool return values (e.g.,
terminalChanged, browserChanged), and include those flags in the progress
determination along with the existing pending-state checks.
- Around line 8035-8049: The scheduled follow-up callbacks use
DispatchQueue.main.asyncAfter without a cancellable work item, so create and use
a DispatchWorkItem for these timeouts: allocate a DispatchWorkItem, assign it to
the existing layoutFollowUpTimeoutWorkItem (or a new clearly named property if
another asyncAfter is used), cancel any previous work item before assigning,
schedule it with DispatchQueue.main.asyncAfter(deadline: .now() + delay,
execute: workItem), and ensure clearLayoutFollowUp continues to cancel and nil
out layoutFollowUpTimeoutWorkItem; apply the same pattern to the other
asyncAfter usages referenced (the follow-up attempt scheduling range) so all
scheduled callbacks are cancellable across follow-up generations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94dea376-a212-4c8b-b72a-fd790b5c4d92
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftSources/Workspace.swift
5 tasks
This was referenced Mar 23, 2026
This was referenced Mar 28, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
5 tasks
This was referenced May 12, 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
Workspacelayout follow-up scheduling so observers do not enqueue an unbounded number of main-queue passesTesting
./scripts/reload.sh --tag fix-1628-layout-followupFixes #1628.
Summary by cubic
Fixes a layout follow-up spin loop by deduping attempts, adding backoff when no progress is made, and avoiding redundant portal refreshes. Stabilizes focus and portal updates to stop unbounded main-queue passes. Fixes #1628.
debugPortalActivegetter to avoid unnecessarysetActivecalls.Written for commit 3c2ad9f. Summary will update on new commits.
Summary by CodeRabbit