test(focus-recovery): start the hidden/tiny reveal from a hidden panel - #14060
Merged
Merged
Conversation
automaticApplyDoesNotBypassHiddenTinyFirstResponderDeferral and findTerminalRestorePreservesHiddenTinyFirstResponderDeferral hide the panel early in setup and count on their final setVisibleInUI(true) to schedule the automatic first-responder apply. setVisibleInUI only schedules that apply on a hidden-to-visible transition. CI diagnostics showed the first responder still on the window after the reveal, which fits a reveal that changed nothing because setup had left the panel visible. Both tests now hide the panel right before the reveal and require the hidden and visible states around it. The find test also left the overlay's own focus handling active. Mounting the overlay forces focus into the search field, and the overlay's focus binding starts set and clears only when the field ends editing. The test now waits for the field to mount, cancels the forced retries, lets the field take and resign focus, and then sets the terminal focus intent it exercises. Both first-responder expectations now print the actual first responder when they fail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
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; 0 remain after this review. 📝 WalkthroughWalkthroughThe focus recovery tests now establish hidden-panel state before revealing the panel. The find-overlay test also mounts a search field, checks its focus, and clears its focus claim before the reveal. ChangesFocus recovery test coverage
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable risk remains identified in these test changes. The reported E2E cleanup error remains, but the supplied evidence does not attribute it to this PR. 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches 💡 1📝 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 |
This was referenced Sep 24, 2026
Collaborator
|
Closing my #14100 in favour of this PR, which fixes the same two tests and is stronger: it uses |
teamleaderleo
added a commit
that referenced
this pull request
Sep 24, 2026
The focused macOS 26 run of fa8d583 failed globalSearchRoutesWhileCommandPaletteIsEffective on the same inactive host the chord test already works around: routing reached the palette, but the popover could not present. Both command-palette routing tests now count requests through the same substitute menu bar extra, which now installs and restores itself. The same run showed that activating the app host does not help the hidden/tiny focus-recovery tests: NSApp.isActive never became true in 10 seconds. That repair is reverted here; #13948 and #14060 own those tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Sep 24, 2026
* fix: keep the checklist popover across a same-turn row reparent On macOS 26 the native popover closes while a sidebar row is reparented synchronously. The section now tells a positioning-view detach from a real click-away: if the same workspace reattaches in the same turn with presentation still requested, the popover is presented again without writing presented=false. A presentation generation retires the deferred close so a reused or unmounted cell cannot write back to the wrong workspace, and unmount writes back a close that is still deferred. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: repair the app-host suites that fail only on macOS 26 Shards 2, 4, 6 and 7 of the PR app-host lane now run on macOS 26, where these tests failed on every run while passing on macOS 15: - The hidden and tiny terminal-focus recovery tests activate the app host before asking AppKit for real first-responder ownership. - The Global Search chord test counts palette requests through a test-owned menu bar extra controller instead of waiting for a popover that an inactive xcodebuild host cannot present. - The Campfire hook mock server serves an accepted client on its own server task instead of dispatching back onto the contended global pool. - The glass-chrome test sets the backdrop settings to match its useGlass argument, so the bound terminal keeps the native glass root. - The geometry feedback-loop canary puts its rows in a ScrollView so the growing rows cannot resize the window and crash the test host. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: let a checklist popover that survived a reparent dismiss on click-away The detach flag set in viewWillMove(toWindow: nil) was only cleared by a deferred close, unmount, or detach. When the popover stayed open across the reparent, the flag stayed set, and the next real click-away took the deferred path and presented the popover again. The section now clears the flag once it is back in a window with the popover still shown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: count Global Search requests in the command-palette routing tests The focused macOS 26 run of fa8d583 failed globalSearchRoutesWhileCommandPaletteIsEffective on the same inactive host the chord test already works around: routing reached the palette, but the popover could not present. Both command-palette routing tests now count requests through the same substitute menu bar extra, which now installs and restores itself. The same run showed that activating the app host does not help the hidden/tiny focus-recovery tests: NSApp.isActive never became true in 10 seconds. That repair is reverted here; #13948 and #14060 own those tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: retire a stale checklist detach flag on new presentation and reuse A detach flag left by a previous session could turn the next real click-away into a deferred re-present, and a deferred close could latch the dismiss ack on the workspace a reused cell now shows. Clear the flag when a presentation starts, and on workspace reuse write the old workspace back and advance the generation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: give the first Search popover time to present on macOS 26 Run 35983669007 failed only the first local-monitor-chain test: the popover did not appear within the 2 s wait, while every later popover test in the same app host presented and passed. The wait returns as soon as the window appears, so a 10 s ceiling costs nothing when it passes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: retry the Search popover while a fresh status item settles The 10 s wait in 37bb6b7 did not help: run 35986561898 failed the same first local-monitor-chain test, and the log shows AppKit rejecting the popover's geometry (x and y infinity) right after the toggle. The routing tests before it now swap the menu bar extra, so this test is the first to anchor on a newly created status item, which macOS 26 places asynchronously. The popover then counts as shown but never appears, and a second toggle would only close it. The helper now closes that phantom and toggles again, up to three times, with a 3 s wait per attempt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
teamleaderleo
enabled auto-merge (squash)
September 24, 2026 18:30
rustybret
pushed a commit
to rustybret/bmux
that referenced
this pull request
Sep 24, 2026
7f58f60 ci: seed the SwiftPM manifest cache for the macOS 15 pool (manaflow-ai#14272) eb8c210 ci: start macOS checkouts from main's git objects (manaflow-ai#14255) 63f485d ci: count the unseeded macOS 15 pool's cold compile when picking a PR pool (manaflow-ai#14268) 5ecf3dd fix(ios): keep alternate-screen apps within the visible viewport (manaflow-ai#12844) 87946bb fix(ios): accept the Mac's push key-exchange reply so pushes decrypt (manaflow-ai#14267) 925c73f ci: fix owned pool review items before the fleet is switched on (manaflow-ai#14252) 1d2a786 test(focus-recovery): start the hidden/tiny reveal from a hidden panel (manaflow-ai#14060) 863f636 Fix BETA signing and prepare iOS migration candidates (manaflow-ai#14265) # Conflicts: # .github/workflows/ci-guards.yml # .github/workflows/ci-macos.yml # .github/workflows/ci-owned-pool-rescue.yml # .github/workflows/ci-queue-janitor.yml # .github/workflows/ci.yml # .github/workflows/cli-pipe-regressions.yml # .github/workflows/ios-testflight.yml # .github/workflows/remote-daemon.yml # .github/workflows/seed-derived-data.yml # .github/workflows/seed-swiftpm-manifests.yml
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
2 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
Two focus-recovery tests fail on main, and the cause is their fixtures. On main's full-suite run 35903570794:
WorkspaceTerminalFocusRecoverySwiftTests/automaticApplyDoesNotBypassHiddenTinyFirstResponderDeferral()fails at lines 325, 326 and 338.WorkspaceTerminalFocusRecoverySwiftTests/findTerminalRestorePreservesHiddenTinyFirstResponderDeferral()fails at lines 413 and 425.Both tests hide and shrink the terminal, then call
setVisibleInUI(true)and expect the automatic first-responder apply to put focus back on the terminal while Ghostty focus stays deferred.GhosttySurfaceScrollView.setVisibleInUIschedules that apply only on a hidden-to-visible transition. In CI the first responder was still the window after the reveal, which is what a reveal of an already-visible panel produces. I did not isolate which setup step leaves the panel visible. Both tests now hide the panel right before the reveal, and#requirethat it is hidden before the reveal and visible after it. A setup change that makes the reveal a no-op again now fails at that#require.The find test had a second problem: the find overlay's own focus handling was still active. Mounting the overlay forces focus into its search field, and the overlay's focus binding stays set until the field ends editing. The test now waits for the field to mount, re-applies the overlay state to cancel the mount's forced focus retries, and lets the field take focus and resign it. It then sets the panel's focus intent to the terminal, which is the restore the test is named for.
Both first-responder expectations now print the actual first responder when they fail. No product code changes and no assertion is relaxed.
This covers two of the five tests that #14006 lists as not covered. Of the other three, #13968 (merged) fixed the key-down recovery test, #14049 fixes the split feedback test, and #14058 fixes the minimal-mode test. Refs #13879.
Testing
Nothing was built or run locally. Hosted runs:
The E2E workflow ends red on every run. Its cleanup step exits 1 after the tests with "refusing to inspect app hosts outside runner temp". The results above come from the test log.
Localization audit: no user-facing strings changed.
Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit
Current results (merged with main at 804e16d)
WorkspaceTerminalFocusRecoverySwiftTestspass, including both tests this PR fixes.findTerminalRestorePreservesHiddenTinyFirstResponderDeferralpasses.automaticApplyDoesNotBypassHiddenTinyFirstResponderDeferralstill fails at line 335, because Ghostty desired focus turns true while the surface is tiny.hiddenTinyFirstResponderReappliesGhosttyFocusAfterGeometrySettlesfails the same way, andforcedReparentFocusClearRetriesWhenSurfaceGeometryIsStillTinyfails intermittently. main fails the same tests on macOS 26 (run 36037832075), so this PR is not the cause. That macOS 26 run replaces the earlier 35832271355 claim above; neither test is fixed on macOS 26.scripts/ci/app-host-known-failures.jsonstay until macOS 26 passes.🤖 Generated with Claude Code