Repository navigation
Fix terminal first render for Cmd+N workspaces - #3264
Closed
austinywang wants to merge 11 commits into
Closed
austinywang wants to merge 11 commits into
austinywang wants to merge 11 commits into
Conversation
The current blank-terminal reports are centered on the user shortcut path, not the background socket-create path, so the regression test drives Cmd+N and requires the newly selected workspace to present a frame without any follow-up tab switch. Constraint: Repository policy requires a failing regression test commit before the fix commit Rejected: Reuse workspace.create background tests | they bypass the focused shortcut/selection handoff that reproduces issue 3068 Confidence: medium Scope-risk: narrow Directive: Keep this test on the real shortcut path; socket-driven workspace.create does not cover the same rendering lifecycle Tested: Not run locally per repository policy Not-tested: Whether current main fails deterministically in every local environment
… ready New workspaces and restored launch-selected workspaces start visible from their first SwiftUI render, so they never pass through the existing visible=false -> true portal refresh path. Reusing that redraw nudge when the Ghostty runtime surface reports ready forces the first frame without waiting for a later tab switch. Constraint: Must avoid sleep-based app timing fixes and keep the change off the typing hot path Rejected: Re-sequence TabManager.addWorkspace publishes | broader behavior risk without proving the publish burst is the only missing signal Rejected: Force portal sync from addWorkspace with delayed async hops | less direct than using the runtime-surface-ready callback that already marks attach readiness Confidence: medium-high Scope-risk: narrow Directive: Keep first-frame refresh keyed to real lifecycle signals; do not add timer-based retries here Tested: git diff --check; python3 -m py_compile tests_v2/test_workspace_shortcut_initial_render.py Not-tested: Local app run and UI regression test execution per repository policy
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds immediate-render triggers so terminal surfaces render their first Metal frame when they become ready or when a workspace/tab is reselected; also adds two end-to-end regression tests that verify initial render after workspace creation and after workspace switching. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TabManager
participant Workspace
participant GhosttyTerminalView
participant WindowPortal
participant MetalLayer
User->>TabManager: create/select workspace (cmd+n / select)
TabManager->>Workspace: set selectedTabId / focusSelectedTabPanel
Workspace->>Workspace: scheduleSelectionRenderFollowUp(panelId)
Workspace->>GhosttyTerminalView: notify selection / layout follow-up
GhosttyTerminalView->>WindowPortal: terminalSurfaceDidBecomeReady
alt portal visible & attached
GhosttyTerminalView->>WindowPortal: refreshSurfaceNow
WindowPortal->>MetalLayer: present first frame
MetalLayer-->>GhosttyTerminalView: presentCount++
else portal not attached
GhosttyTerminalView-->>Workspace: await attach/visibility
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Contributor
Greptile SummaryFixes two closely related blank-first-frame gaps for terminal workspaces: a brand-new
Confidence Score: 5/5The changes are well-scoped, reuse existing patterns, and both fix paths are covered by the new regression tests. The two production Swift changes are conservative: the refreshSurfaceNow addition reuses the same redraw nudge already used in setVisibleInUI, and scheduleSelectionRenderFollowUp delegates entirely to the established beginEventDrivenLayoutFollowUp machinery. The PortalRenderingPhase enum refactor changes the initial state from enabled to disabled intentionally, causing the first setPortalRenderingEnabled(true) call to always trigger a layout follow-up instead of silently skipping it. No new timing primitives, sleeps, or actor-isolation issues were introduced. Sources/Workspace.swift — the silent guard drop in scheduleSelectionRenderFollowUp for unmounted workspaces is worth a second read, though behavior is still an improvement over pre-PR. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant TabManager
participant Workspace
participant GhosttyTerminalView
participant TerminalSurface
Note over User,TerminalSurface: Path 1 — Brand-new Cmd+N workspace
User->>TabManager: Cmd+N shortcut
TabManager->>Workspace: focusPanel(panelId)
TabManager->>Workspace: "scheduleSelectionRenderFollowUp(panelId) [no-op: portalRenderingEnabled=false]"
Workspace-->>ContentView: SwiftUI renders new tab
ContentView->>Workspace: setPortalRenderingEnabled(true) → beginEventDrivenLayoutFollowUp (geometry)
TerminalSurface-->>GhosttyTerminalView: terminalSurfaceDidBecomeReady notification
GhosttyTerminalView->>GhosttyTerminalView: "isVisibleInUI && window!=nil && !isHidden"
GhosttyTerminalView->>TerminalSurface: refreshSurfaceNow(surfaceDidBecomeReady) ✅
Note over User,TerminalSurface: Path 2 — Reselecting an existing (mounted) workspace
User->>TabManager: select workspace
TabManager->>Workspace: focusPanel(panelId)
TabManager->>Workspace: "scheduleSelectionRenderFollowUp(panelId) [portalRenderingEnabled=true]"
Workspace->>Workspace: beginEventDrivenLayoutFollowUp replaceSelectionTargets + terminalFocusPanelId
Workspace->>GhosttyTerminalView: setVisibleInUI(true)
GhosttyTerminalView->>TerminalSurface: refreshSurfaceNow(setVisibleInUI) ✅
Reviews (7): Last reviewed commit: "Fix workspace portal mount ownership" | 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests_v2/test_workspace_shortcut_initial_render.py`:
- Around line 67-89: The readiness predicate in _wait_for_surface_mount is too
strict and the short first-present timeout causes CI flakes: update the ready()
checks in _wait_for_surface_mount to stop requiring surface_focused and relax
the hosted_view_frame size check (use a smaller threshold, e.g. width and height
>= 40.0 instead of 80.0), and increase the first-present/initial timeout used
elsewhere from 2s to a more forgiving value (e.g. 5s) so loaded runners are not
marked failed prematurely; touch the readiness boolean expressions referencing
surface_focused, hosted_view_frame width/height, and the code that defines the
initial timeout to apply these changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a631a5c3-f75b-4034-88c4-7159090a05f9
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swifttests_v2/test_workspace_shortcut_initial_render.py
…rame The remaining blank-terminal reports are not limited to brand-new workspaces; several reports say an already-existing workspace comes back blank on the first switch and only recovers after another switch. This regression test warms two workspaces, alternates between them, and requires each first return to present a new frame without manual recovery. Constraint: Repository policy requires a failing regression test commit before the fix commit Rejected: Extend the Cmd+N-only test | it does not cover already-created surfaces returning from an inactive workspace Confidence: medium Scope-risk: narrow Directive: Keep this test on workspace selection/remount, not background creation, because the bug depends on returning an existing portal-hosted surface Tested: Not run locally per repository policy; python3 -m py_compile tests_v2/test_workspace_switch_initial_render.py Not-tested: Deterministic failure rate on every local machine before the follow-up fix
Brand-new visible terminals were only part of the blank-screen family. Existing workspaces could also return blank on the first switch because workspace selection remounted portal-hosted views without starting the workspace-level follow-up loop that reconciles terminal and browser portal visibility after attach. Routing workspace selection through the existing follow-up machinery gives remounted panels a post-attach geometry/visibility pass on the first return instead of waiting for a second switch, and it does so through the shared workspace path that already knows how to reconcile both terminal and browser portals. Constraint: Must preserve the existing portal/lifecycle model and avoid adding timer-only redraw hacks Rejected: Add another terminal-only refresh callback | too narrow for the broader existing-workspace and browser-pane symptom family Rejected: Rework workspace mount policy or published selection sequencing | much broader risk surface than starting the existing reconciliation loop at selection time Confidence: medium-high Scope-risk: narrow Directive: Keep workspace selection fixes on the shared follow-up path so terminal and browser portal behavior stays aligned Tested: git diff --check; python3 -m py_compile tests_v2/test_workspace_shortcut_initial_render.py tests_v2/test_workspace_switch_initial_render.py Not-tested: Local app run and UI regression execution per repository policy
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Workspace.swift`:
- Around line 11641-11650: The follow-up targets can linger because
beginEventDrivenLayoutFollowUp only updates targets when non-nil; fix by adding
a way to force-clearing stale targets and call it from
scheduleSelectionRenderFollowUp: change beginEventDrivenLayoutFollowUp to accept
a flag like forceResetTargets: Bool (or an explicit resetTargets parameter) and
implement logic that clears stored browser/terminal targets when
forceResetTargets is true; then call beginEventDrivenLayoutFollowUp(reason:
"workspace.selection", browserPanelId: browserPanelId, terminalFocusPanelId:
terminalPanelId, includeGeometry: true, forceResetTargets: true) from
scheduleSelectionRenderFollowUp (referencing scheduleSelectionRenderFollowUp,
browserPanel(for:), terminalPanel(for:), and beginEventDrivenLayoutFollowUp).
In `@tests_v2/test_workspace_switch_initial_render.py`:
- Line 24: The default SOCKET_PATH assignment uses a fixed path which can
collide; change the fallback for os.environ.get("CMUX_SOCKET",
"/tmp/cmux-debug.sock") to a user-scoped or unique path (e.g., include the
current UID or username or use tempfile.gettempdir() plus a per-user identifier)
or make CMUX_SOCKET required; update the SOCKET_PATH variable initialization to
compute a fallback such as
f"{tempfile.gettempdir()}/cmux-debug-{os.getuid()}.sock" (or similar) so
concurrent users/sessions don't collide.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1bdf2442-1422-4044-85d6-860530b9ed34
📒 Files selected for processing (3)
Sources/TabManager.swiftSources/Workspace.swifttests_v2/test_workspace_switch_initial_render.py
Selection-driven layout follow-up now replaces stale terminal/browser targets instead of inheriting pending work from a previous selected panel. The new regression harnesses also wait for the render condition they actually assert and use the tagged socket path exported by the runner. Constraint: PR feedback flagged stale follow-up targets and CI-flaky readiness gates. Rejected: Adding another timing repair in Workspace | would preserve split ownership of pending layout targets. Confidence: high Scope-risk: narrow Directive: Selection changes should derive follow-up targets from the newly selected panel and clear prior target IDs. Tested: python3 -m py_compile tests_v2/test_workspace_shortcut_initial_render.py tests_v2/test_workspace_switch_initial_render.py; git diff --check Not-tested: Local app/E2E tests; repo policy runs tests in CI/VM only.
Selection render follow-up should replace the targets derived from the selected panel, not unrelated pending focus-restoration work. The target update policy now preserves the split-zoom browser-exit focus target while still clearing stale terminal/browser selection targets. Constraint: Greptile flagged that replace-all target semantics could drop split-zoom-exit focus restoration in the same run-loop cycle. Rejected: Clearing every pending target on selection change | selection does not own split-zoom-exit focus restoration. Confidence: high Scope-risk: narrow Directive: Keep layout follow-up target replacement scoped to the source that owns the target. Tested: git diff --check Not-tested: Local app/E2E tests; repo policy runs tests in CI/VM only.
Selection-driven layout follow-up now carries the selected panel ID, replacing selection-owned targets while preserving only same-panel split-zoom exit focus intent. This prevents stale browser-exit focus work from overriding a new selection without dropping legitimate same-panel exit recovery. Constraint: Review feedback identified both over-clearing and under-clearing risks for browser split-zoom exit focus restoration. Rejected: Always preserve browser-exit focus intent | stale intent can refocus the wrong panel after selection changes. Rejected: Always clear browser-exit focus intent | same-panel split-zoom exit recovery can be lost. Confidence: high Scope-risk: narrow Directive: Layout follow-up target replacement must compare ownership against the selected panel before clearing cross-purpose pending work. Tested: git diff --check Not-tested: Local app/E2E tests; repo policy runs tests in CI/VM only.
Start workspace portal rendering from an explicit unmounted phase so the first selected mount always transitions through the shared layout follow-up owner. This removes the implicit enabled-at-init state that could let initial render reconciliation depend on selection/focus callback ordering. Constraint: Do not run reload, xcodebuild, or local tests during the bulk run. Rethink: make the Workspace MainActor model own portal mount phase instead of treating first mount as an already-enabled boolean state. Swift guidance: keep lifecycle state synchronous in the model and avoid adding timer/sleep repair paths. Tested: git diff --check Not-tested: local app build, xcodebuild, reload, and local tests per user constraints.
lawrencecchen
added
the
stale-revisit
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
3 participants
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
Cmd+Nworkspace-creation path that requires the newly selected workspace to present a terminal frame without any follow-up tab switchvisible=false -> trueportal visibility restoresReferences:
Root cause
There are two closely related first-visible-frame gaps here.
Brand-new visible terminal path
GhosttySurfaceScrollView.setVisibleInUI(_:)already callsrefreshSurfaceNow(...)when a terminal becomes visible again after being hidden, which is why switching away and back immediately recovers the blank view. But a brand-new selected workspace starts out visible on its first SwiftUI render, so it never passes through thatfalse -> truevisibility transition.On that path, the runtime Ghostty surface becomes ready,
terminalSurfaceDidBecomeReadyfires, and the code only re-ran first-responder focus handling. There was no equivalent visible redraw nudge at surface-ready time, so the first Metal frame could remain blank until a later tab/workspace switch toggled visibility and finally hit the existing refresh path.Existing workspace remount / first-return path
Several user reports are broader than brand-new workspaces: an already-existing workspace comes back blank on the first switch and only recovers after another switch. The shared workspace-level follow-up loop is the path that reconciles terminal and browser portal visibility after attach/remount, but workspace selection did not start that loop at all.
That meant a reselected workspace could remount portal-hosted views and restore focus without ever running the shared post-attach reconciliation pass until some later interaction caused a second visibility/layout churn.
Reproduction
Cmd+Nin the main window.The same family also explains broader reports where returning to an existing workspace blanks on the first switch but recovers on a second switch, and why browser panes have shown a similar “white until another selection/layout churn” symptom.
Regression tests
tests_v2/test_workspace_shortcut_initial_render.pytests_v2/test_workspace_switch_initial_render.pyThe first test drives the real
Cmd+Nshortcut path instead of socket backgroundworkspace.create, creates a burst of workspaces to amplify the attach timing churn, and asserts that the final selected workspace presents at least one terminal frame without any manual tab switch, typing, orrefresh-surfacesworkaround.The second test warms two existing workspaces, switches between them repeatedly, and requires each first return to present a new frame without a second switch. That locks the broader remount/reattach path covered by user reports outside the brand-new-workspace case.
The commits keep the repository’s failure-before-fix structure:
8f030400test only for new visible workspaces3eb2c00ffix for runtime-surface-ready visible redrawa8d3b6b5test only for existing-workspace first return5737b45bfix for workspace-selection follow-up reconciliationIssue relationship
2555also includes longer-running-session and browser-specific reports that may still have additional causes.Nightly status
At the time of this update, current
origin/main/ nightly does not contain this fix stack. The branch includes commits not present onorigin/main, so reproducing on current nightly is expected unless another unrelated fix lands separately.Testing
git diff --check.python3 -m py_compile tests_v2/test_workspace_shortcut_initial_render.py tests_v2/test_workspace_switch_initial_render.py.Note
Medium Risk
Touches workspace selection and portal/layout follow-up scheduling, which can affect focus/visibility timing across terminals and browser panels, but is limited to UI rendering behavior with added regression coverage.
Overview
Fixes a blank-first-frame regression where newly created or reselected workspaces could show mounted terminal/browser portals without presenting an initial frame until a second workspace/tab switch.
When a
terminalSurfaceDidBecomeReadynotification arrives for a visible terminal,GhosttyTerminalViewnow triggers an immediaterefreshSurfaceNowto nudge the first Metal frame. Workspace selection also now starts a dedicated selection render follow-up (scheduleSelectionRenderFollowUp) so remounted portal-hosted views get an immediate post-attach reconciliation pass.Adds two
tests_v2regressions that drive the real Cmd+N creation path and repeated workspace switching, assertingpresentCountadvances without any extra user interaction.Reviewed by Cursor Bugbot for commit 51ab429. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests
Closes #3068