Repository navigation
Apply render suspend to canvas terminals - #6979
austinywang wants to merge 16 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughCanvas terminal mounting now toggles renderer portal visibility on render changes and hides through the hosted view on unmount. The terminal visibility tests moved to ChangesRenderer portal lifecycle and test migration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ 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 |
e570e10 to
3fb2d87
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/GhosttyTerminalViewVisibilityPolicyTests.swift`:
- Around line 70-93: The canvas terminal rendering test currently only verifies
portal visibility, so it can miss regressions where renderer realization is
dropped. Update the GhosttyTerminalViewVisibilityPolicyTests test around
CanvasPaneContentMount and TerminalPanel.surface to also assert
panel.surface.isRendererRealized after setRendering(true) and after unmount(),
while keeping the existing isRendererPortalVisible checks. Ensure the assertions
cover the transitions driven by setRendering(_:) and unmount() so both
visibility and realization behavior are validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e46c62df-1b4b-4fbf-b522-facd7b5970b0
📒 Files selected for processing (2)
Sources/Canvas/CanvasPaneContent.swiftcmuxTests/GhosttyTerminalViewVisibilityPolicyTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/GhosttyTerminalViewVisibilityPolicyTests.swift`:
- Around line 89-90: The render-suspend test in
GhosttyTerminalViewVisibilityPolicyTests only verifies portal visibility after
mount.setRendering(false), so it can miss a bug where the renderer stays
realized while hidden. Update the existing assertion block to also check
panel.surface.isRendererRealized is false, using the same mount and
panel.surface symbols, so the test covers the suspend half of the contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c0ecd95-ebf8-4a5c-890b-6af32e6967b3
📒 Files selected for processing (1)
cmuxTests/GhosttyTerminalViewVisibilityPolicyTests.swift
Assert that CanvasPaneContentMount.unmount() leaves the terminal surface portal-hidden so RendererRealizationController can reclaim its GPU renderer. This fails against the current unmount(), which pins the surface visible — the fix follows in the next commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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/GhosttyTerminalViewVisibilityPolicyTests.swift">
<violation number="1" location="cmuxTests/GhosttyTerminalViewVisibilityPolicyTests.swift:83">
P1: The test asserts that after `mount.unmount()`, `panel.surface.isRendererPortalVisible` is `false` (portal-hidden), which is correct per the comment — a backgrounded canvas tab must report portal-hidden so `RendererRealizationController` doesn't skip `releaseRenderer()` and leak the GPU renderer. However, the current production `CanvasPaneContentMount.unmount()` explicitly calls `panel.surface.setRendererPortalVisible(true)`. If this PR doesn't also change `unmount()` to set portal-visible to `false`, the test will fail and the regression isn't covered. Please verify the companion production change is present.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… be reclaimed CanvasPaneContentMount.unmount() pinned the terminal surface visible (setRendererPortalVisible(true) + realizeRenderer + setOcclusion(true)). unmount() also runs when a canvas tab is deselected via CanvasRootView.reconcileMount, where no portal re-hosts the surface, so the surface stayed "visible forever": RendererRealizationController's releaseRenderer() guards on !rendererPortalVisible and skipped it on every pass, leaking one GPU renderer (Metal swap chain / IOSurface) per backgrounded canvas tab. Hand the surface off in the same hidden state as the authoritative portal hide (GhosttySurfaceScrollView.setVisibleInUI(false)): portal-hidden and occluded, without realizing. This lets the controller reclaim the renderer on its idle policy, and leaving occlusion off avoids Ghostty drawing into a swap chain the controller has since released. When the split path re-hosts on its next update, setVisibleInUI(true) re-marks the surface visible and re-realizes before any draw; release stays controller-driven. Fixes the failing test added in the previous commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Commit 2 poked TerminalSurface directly (setRendererPortalVisible(false) + setOcclusion(false)) but never updated GhosttySurfaceScrollView's inner `visibleInUI` flag. That desyncs the portal: when the split re-hosts the surface and calls setVisibleInUI(true), it reads `wasVisible = surfaceView.isVisibleInUI` == true, so `wasVisible != visible` is false and setOcclusion(true) is skipped — the re-shown split terminal stays occluded and frozen. Hide through the shared authoritative path (GhosttySurfaceScrollView .setVisibleInUI(false)) instead. It flips `visibleInUI` to false, marks the surface portal-hidden (so RendererRealizationController can reclaim its GPU renderer for a backgrounded canvas tab) and occluded, and does not realize on hide. Re-hosting then performs a real false→true transition that re-realizes and re-occludes. Renderer release stays controller-driven. Addresses the cubic review finding on the occlusion desync; keeps the regression test from the first commit green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…weight-render-suspend-hibernation-t
…weight-render-suspend-hibernation-t
…weight-render-suspend-hibernation-t
…weight-render-suspend-hibernation-t
…weight-render-suspend-hibernation-t
Fixes #5497
Summary
Validation
Note: per task instructions, I did not run reload.sh or xcodebuild.
Summary by cubic
Suspend canvas terminal rendering when panes leave the render region, and resume by realizing the renderer when they re-enter. Fixes #5497 by stopping offscreen draws, preventing GPU leaks on deselected canvas tabs, and avoiding frozen terminals on re-host.
Bug Fixes
Tests
Written for commit 8760ed8. Summary will update on new commits.
Summary by CodeRabbit
Testing.