test: give the Cloud Desktop fixture a routable window for pane drops - #14304
Merged
Merged
Conversation
"A queued Desktop click retains its same-VM destination like a drop" fails its drop on main (every case since run 35979983434): the catalog keeps one projection and the commit wait times out after 10s. The drop targets `.split(targetPane:)`. SurfacePaneFactory resolves that pane's anchor through TerminalController.v2LocatePane, which walks listMainWindowSummaries. That list skips any registered context without a window, and VaultPaneAppFixture registers its context with `window: nil`. So the lookup returns nil, the factory throws paneNotFound, and handleSurfaceResourceDrop's Task swallows the error. The click path is unaffected because it targets `.workspace(_, .split)` and never locates a pane. The same suite failed 4/4 run on its own in #14076's changed-suites lane and hung once on main before that (run 35862070143), so it depended on test order rather than on a product change. Give the fixture's context a task-owned NSWindow that is never shown, forget its route on close, and require the pane lookup before the drop so a routing failure is reported where it happens. 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. Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
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 (3)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change exposes ChangesCloud desktop open fixture
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to The fixture’s hidden-window routing and cleanup show no identified merge-blocking issue. The changed suites still need to run as planned. 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (24 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files. (1 skipped: 1 too large.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
With a routable window, the opens and drops in this suite request focus, and focusMainWindow now resolves the fixture window and would order it front and make it key on a shared app-host runner. Stub the fixture delegate's visibility controller with activation suppressed, as #13938 does, which needs the property to be internal instead of private, and expect the window to stay hidden and not key around every action. Also require the drop to be accepted and the located route to match the fixture's manager and pane. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
teamleaderleo
enabled auto-merge (squash)
September 24, 2026 23:13
With the fixture fixed, capturesClickDestination must pass instead of being tolerated as a known main failure, so this PR's changed-suites run proves the fix rather than passing either way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Swift rejects a method call on self before every stored property is set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rustybret
pushed a commit
to rustybret/bmux
that referenced
this pull request
Sep 25, 2026
3d5b648 test: give the Cloud Desktop fixture a routable window for pane drops (manaflow-ai#14304) 9d53af4 ci: give a refused owned job one more try on the fleet before Blacksmith (manaflow-ai#14312) 379091b ci: let PR runs overflow to macOS 15 at 4 queued jobs deeper, not 12 (manaflow-ai#14319) df6efb5 test(cloud): key the refresh URL protocol stub per request, not by address (manaflow-ai#14239) 51bd322 ci: build only the CLI product for CLI-only changes (manaflow-ai#14212) 0345a5c ci: make a changed-suites run prove a known-failure fix (manaflow-ai#14307) 370b7f6 ci: fix the owned build state save step's argument count (manaflow-ai#14309) 319adff test: wait for the SSH cleanup policy bound after a restored-attach signal (manaflow-ai#14305) 0cc5be3 fix(portal): flush the coalesced live-resize pass on its first hop (manaflow-ai#14297) 5887891 test(minimal-mode): measure the toggle only after setup stops re-rendering (manaflow-ai#14298) 5fbcc48 ci: charge newer PR runs what they took on the owned pool, not a guess (manaflow-ai#14300) # Conflicts: # .github/workflows/ci-macos.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
1 participant
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.
"A queued Desktop click retains its same-VM destination like a drop" fails its drop on main in all four argument cases (every main run since 35979983434, including PR run 36023014704): the catalog keeps one projection and
CloudDesktopOpenFixture.swift:155didCommitis false.The drop targets
.split(targetPane:).SurfacePaneFactory.anchorSurfaceresolves that pane throughTerminalController.v2LocatePane, which walkslistMainWindowSummaries, and that list skips a registered context without a window.VaultPaneAppFixtureregisters its context withwindow: nil, so the lookup returns nil, the factory throwspaneNotFound, andhandleSurfaceResourceDrop's Task swallows the error. The click half of the test targets.workspace(_, .split), never locates a pane, and still passes.This is a fixture gap, not a product regression: no routing or factory code changed. The suite failed 4/4 when #14076's changed-suites lane ran it alone (job 107549099765, reported green because that lane does not gate), and it hung once on main before #14076 (run 35862070143). Earlier full shards passed it, so the outcome depended on what ran before it.
The fixture now gives its context a task-owned
NSWindow, forgets the route and closes the window on teardown, and requires the pane lookup before dropping so a routing failure is reported at the lookup instead of after the 10 s commit deadline. Once the window is routable, the suite's opens and drops would focus it (focusMainWindow->makeKeyAndOrderFront), so the fixture's own delegate gets a visibility controller with activation suppressed, and the suite expects the window to stay hidden and not key around every action. That stub needsAppDelegate.mainWindowVisibilityControllerto be internal instead of private; nothing else inSources/changes. #13938 makes the same registration and the same one-word access change as part of a larger PR; this lands the test fix alone.It also removes the test from
scripts/ci/app-host-known-failures.json(tracked in #13879), so the changed-suites ratchet requires it to pass instead of tolerating it. That tolerance is why #14076's lane went green with the suite failing.Validation: not built on this Mac. This diff routes
CloudDesktopOpenActionTestsinto the app-host changed-suites lane; that run is the proof.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes the Cloud Desktop fixture so "A queued Desktop click retains its same-VM destination like a drop" passes on main instead of timing out.
The drop's
v2LocatePanelookup returned nil because the fixture registered its context without a window, and the drop's Task swallowed the resultingpaneNotFounderror. The fixture now gives its context a task-ownedNSWindowthat is never shown, stubs the visibility controller with activation suppressed so focus requests can't reveal it, and forgets the route and closes the window on teardown. The suite asserts the window stays hidden and never key after every action, including right after fixture init, anddroprequires the pane lookup, route, and drop acceptance to match the fixture before proceeding.capturesClickDestinationis removed from the known-failure catalog so the changed-suites run must actually pass. This is a fixture gap, not a product regression.Written for commit 77b1305. Summary will update on new commits.
Summary by CodeRabbit