Fix #1357: hide stale terminal portal after restore churn - #2025
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
To use Codex here, create a Codex account and connect to github. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModified TerminalWindowPortal.synchronizeHostedView(withId:) to defer hiding a visible hosted view when its anchor or window is temporarily unavailable by scheduling a transient-recovery retry and returning early; added a regression test that verifies deferred sync hides retired hosted views and clears stale hit-testing regions. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Portal as TerminalWindowPortal
participant Anchor as AnchorView/Window
participant Scheduler as TransientScheduler
participant Hosted as HostedView
Caller->>Portal: synchronizeHostedView(withId)
Portal->>Anchor: check anchorView / window
alt anchor/window missing
Portal->>Hosted: check hostedView.isHidden
Portal->>Scheduler: scheduleTransientRecoveryRetryIfNeeded(...)
alt scheduler scheduled && hosted visible
Portal->>Portal: log "portal.hidden.deferKeep"
Portal-->>Caller: return (defer hide)
else
Portal->>Hosted: hide hosted view
Portal-->>Caller: return (hidden)
end
else anchor/window present
Portal->>Hosted: normal visibility sync / attach
Portal-->>Caller: return
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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 |
![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="cmuxTests/TerminalAndGhosttyTests.swift">
<violation number="1" location="cmuxTests/TerminalAndGhosttyTests.swift:2643">
P2: Avoid fixed 50ms RunLoop sleeps in this async test; they can make the regression test flaky on slower machines. Wait on a condition/expectation instead.
(Based on your team's feedback about avoiding fixed RunLoop sleeps in stale-delay tests.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryThis PR fixes a sidebar bleed issue (#1357) where a terminal portal entry that was still marked Key changes:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant Portal as WindowTerminalPortal
participant Retired as retiredHosted entry
participant Recovery as scheduleTransientRecovery
Note over Caller,Recovery: Restore churn: old anchor removed, new anchor bound
Caller->>Portal: bind(activeHosted, activeAnchor, visibleInUI:true)
Portal->>Portal: synchronizeHostedView(activeHosted)
Portal->>Portal: scheduleDeferredFullSynchronizeAll()
Caller->>Portal: synchronizeHostedViewForAnchor(activeAnchor)
Portal->>Portal: pruneDeadEntries()
Note over Portal: retiredHosted kept because visibleInUI=true
Portal->>Portal: synchronizeAllHostedViews(excluding: activeHosted)
Portal->>Retired: synchronizeHostedView(retiredHosted)
Note over Retired: anchorView==nil, visibleInUI==true
alt transientRecoveryEnabled == false (normal build)
Retired->>Recovery: scheduleTransientRecoveryRetryIfNeeded()
Recovery-->>Retired: false
Note over Retired: shouldPreserveVisibleOnTransient = false
Retired->>Retired: hostedView.isHidden = true
else transientRecoveryEnabled == true (recovery build)
Retired->>Recovery: scheduleTransientRecoveryRetryIfNeeded()
Recovery-->>Retired: true (budget remaining)
Note over Retired: shouldPreserveVisibleOnTransient = true, return early
end
Portal-->>Caller: returns
Note over Portal: DispatchQueue.main.async fires (deferred sync)
Portal->>Portal: synchronizeAllHostedViews(excluding: nil)
Note over Retired: already hidden in normal builds
Reviews (1): Last reviewed commit: "fix: hide stale terminal portal after re..." | Re-trigger Greptile |
![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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TerminalWindowPortal.swift (1)
1214-1247:⚠️ Potential issue | 🟠 MajorDon’t let the final transient retry keep the stale portal visible.
On Line 1215,
scheduleTransientRecoveryRetryIfNeeded(...)still returnstruewhen it decrementstransientRecoveryRetriesRemainingfrom1to0. That makesshouldPreserveVisibleOnTransienttrue, so this branch returns without hiding, but no follow-up sync is queued. The stale terminal can then stay rendered until some unrelated geometry event. The helper call afterisHidden = truealso re-arms the same retry budget again. Please basedeferKeeponentry.transientRecoveryRetriesRemaining > 0after the helper runs, and only invoke the helper once per sync attempt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalWindowPortal.swift` around lines 1214 - 1247, Call scheduleTransientRecoveryRetryIfNeeded(...) only once per sync and base the deferKeep decision on the post-call retry budget: invoke scheduleTransientRecoveryRetryIfNeeded(forHostedId:hostedId, entry:&entry, hostedView:hostedView, reason:"missingAnchorOrWindow") a single time and store its result and then compute shouldPreserveVisibleOnTransient as !hostedView.isHidden && entry.transientRecoveryRetriesRemaining > 0 (not the raw helper boolean which can be true when it just hit 0). Use the stored result to avoid calling scheduleTransientRecoveryRetryIfNeeded again after setting hostedView.isHidden and only schedule/re-arm retries once per sync attempt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/TerminalWindowPortal.swift`:
- Around line 1214-1247: Call scheduleTransientRecoveryRetryIfNeeded(...) only
once per sync and base the deferKeep decision on the post-call retry budget:
invoke scheduleTransientRecoveryRetryIfNeeded(forHostedId:hostedId,
entry:&entry, hostedView:hostedView, reason:"missingAnchorOrWindow") a single
time and store its result and then compute shouldPreserveVisibleOnTransient as
!hostedView.isHidden && entry.transientRecoveryRetriesRemaining > 0 (not the raw
helper boolean which can be true when it just hit 0). Use the stored result to
avoid calling scheduleTransientRecoveryRetryIfNeeded again after setting
hostedView.isHidden and only schedule/re-arm retries once per sync attempt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91468892-8b0c-4999-a910-a03ac0e5754d
📒 Files selected for processing (2)
Sources/TerminalWindowPortal.swiftcmuxTests/TerminalAndGhosttyTests.swift
|
Checked the CodeRabbit note on |
…-terminal-sidebar-bleed Fix manaflow-ai#1357: hide stale terminal portal after restore churn
This branch was successfully deployed
2 participants
Summary
Testing
testDeferredSyncHidesVisibleHostedViewAfterAnchorDisappearsincmuxTests/TerminalAndGhosttyTests.swift../scripts/reload.sh --tag issue-1357-ghost-terminal-sidebar-bleedsuccessfully.Demo Video
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Hide stale terminal portal views when their anchor/window disappears during restore churn to stop ghost terminal content from bleeding into the sidebar (fixes #1357). Clarifies deferred resync in tests to ensure stale views hide and hit regions clear.
Written for commit 8c0aee3. Summary will update on new commits.
Summary by CodeRabbit