Repository navigation
fix(focus): defer every terminal focus reassertion until surface geometry is usable - #14276
Conversation
…etry is usable prepareTerminalSurfaceFocusReassertion only checked geometry when a suppressed reapply was pending or the call was forced. On macOS 26, makeKeyAndOrderFront selects the terminal as the first key view and queues an automatic first-responder apply; when that apply ran after the surface had become hidden or 0x0, it reasserted Ghostty focus anyway. Require usable, visible geometry for every reassertion; the existing path defers and retries once geometry is usable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…requires-geometry
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughTerminal focus reassertions now evaluate portal and surface geometry before proceeding. The early return for non-suppressed, non-forced reassertions was removed. ChangesTerminal focus reassertion
Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to Focus reassertions now wait for usable geometry, and the supplied evidence shows deferred attempts can be retried. No actionable merge-blocking risk remains after normal checks. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (24 passed)
Full details: Cmux Swift Blocking RuntimeExplanation The production focus change materially expands a timing-based main-queue deferral. Before this diff, Resolution Avoid using the main-queue async retry as the geometry synchronization mechanism. Record the pending focus intent, then retry only from an explicit geometry or visibility transition signal, such as the existing layout/visibility reconciliation callbacks, and clear the pending state when that signal confirms usable portal and surface geometry. Do not add polling, delayed dispatch, sleeps, or another timing-based retry.
✨ 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 |
teamleaderleo
added
the
unit-ci
This comment has been minimized.
This comment has been minimized.
…requires-geometry
teamleaderleo
removed
the
unit-ci
1 participant
Terminal focus could be reasserted onto a hidden or 0x0 surface, which is what the macOS 26 failures in
WorkspaceTerminalFocusRecoverySwiftTestscatch (see #13879 and the main full-suite run 36022521492).Cause
GhosttySurfaceScrollView.prepareTerminalSurfaceFocusReassertiononly checked geometry when a suppressed reapply was pending or the call was forced:A call-stack trace on macOS 26 (test-e2e run 36041791369) shows the path:
makeKeyAndOrderFrontruns_setUpFirstResponder→_selectFirstKeyView, which makes the terminal first responder and queues an automatic first-responder apply. When that apply runs from the main queue after the surface has gone hidden or 0x0, it goesapplyFirstResponderIfNeeded→reassertTerminalSurfaceFocus→TerminalSurface.setFocus(true)without a geometry check, so Ghostty is focused while the hidden/tiny deferral is supposed to be pending.Fix
Every reassertion now requires visible, usable portal and surface geometry. When it doesn't have it, it takes the existing deferral path (
pendingSuppressedFirstResponderFocusReapply = trueplus a scheduled retry), which reapplies focus once geometry is usable.Results (test-e2e, the WorkspaceTerminalFocusRecoverySwiftTests suite)
hiddenTinyFirstResponderReappliesGhosttyFocusAfterGeometrySettles,forcedReparentFocusClearRetriesWhenSurfaceGeometryIsStillTiny,findTerminalRestorePreservesHiddenTinyFirstResponderDeferralpass;automaticApplyDoesNotBypassHiddenTinyFirstResponderDeferralpassed in one run and failed in the otherThe remaining
automaticApplyflake looks like a second race: during test setup,TabManager.selectedWorkspaceIdDidChangedispatchesfocusSelectedPanel→Workspace.activatePanel, which callsTerminalSurface.setFocus(true)with no geometry check. If it lands after the test shrinks the surface, Ghostty is focused early. It stays onapp-host-known-failures.json(as do the other entries); this PR does not change that list.Full app-host suite (
unit-ci, run 36049292327, three attempts)Shards 1, 2, 3, 5 and 7 passed. Every failure is unrelated to focus and also fails on main:
tests/test_pi_compacted_feed.py("Pi feed did not report its missing explicit workspace"). fix(cli): fail the Pi feed when an explicit workspace ref stays unresolved #14277 fixes it; this branch predated it and now includes it.VMClientReadCoalescingTests(test(cloud): key the refresh URL protocol stub per request, not by address #14239 is the open fix) and the flakyGhosttySurfaceOverlayTests.testFiveTabRendererFootprint…memory threshold.No focus, split, or TextBox suite failed. The
unit-cilabel is dropped soci-statusis not held on the VMClient suite.🤖 Generated with Claude Code