test: await the geometry publish in the equalize-splits shortcut case - #13916
Conversation
`testConfiguredEqualizeSplitsShortcutBalancesWorkspaceDividers` compared the cached `workspace.tmuxLayoutSnapshot` against the controller's live tree and read the cache synchronously, one statement after driving geometry. Since a27969a the geometry callback publishes that cache through `geometryNotificationScheduler` rather than assigning it in the synchronous `splitTabBar(_:didChangeGeometry:)` body, so both reads saw the previous value -- at the start of a workspace, the one-pane snapshot written at init. The failure reported one pane id against three and frame deltas that are the live equalized geometry (161.33333 = 484/3). Wait for the cache to catch up to the live tree before each read, the way `PaneResizeShortcutTests.expectCachedFramesMatch` already does for the same publication change. The live tree was never wrong: the tree assertions above these reads pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 6 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 (2)
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 equalize-splits shortcut test now runs asynchronously inside the exclusive app-context gate. It waits for the seeded layout to appear and for the cached layout to change before comparing it with the live layout. ChangesLayout Test Synchronization
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to The synchronization change uses predicate-based waiting for the asynchronous layout publication, while equalization itself completes synchronously. No actionable merge-blocking risk remains. 🚥 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 |
|
All contributors have signed the CLA ✍️ ✅ |
teamleaderleo
added
the
full-ci
|
Added Lane needed: |
|
This fix does not work, and my diagnosis was wrong. Do not merge it as-is. The shards ran on One pane id where three are expected. I claimed the cached A Mac repro of unmodified So this PR is treating a symptom. The real question is why Two things worth keeping from it regardless:
I am leaving this open rather than closing it, because the helper and the investigation notes are worth keeping attached to the problem. But it should not land, and nobody should treat — Zarathustra g1 🌱 |
teamleaderleo
changed the title
The previous attempt waited for `tmuxLayoutSnapshot` with
`RunLoop.main.run(mode:before:)`, and CI showed it never succeeding: the
assertion still compared a 1-pane cache against the live 3-pane tree, only
with the line shifted.
`splitTabBar(_:didChangeGeometry:)` hands its work to
`geometryNotificationScheduler.schedule(zeroDelayPolicy: .yieldOnce)`, which
is `Task { await Task.yield(); action() }` on the MainActor executor. A
synchronous `@Test` body owns that executor for its whole duration, so the
continuation cannot run at all — spinning the run loop does not drain the
MainActor's cooperative executor. The cache therefore keeps the one-pane
value written at workspace init, and no timeout value could ever have
worked.
Make the case `async`, wrap it in `AppContextSerialGate.withExclusiveAppContext`
(the pattern this file's other async cases use, since async tests in
different suites interleave at suspension points and this one swaps
`AppDelegate.shared`), and await `Task.yield()` between polls so the
scheduled publish can actually run.
The wait's failure now names the publish rather than returning a stale
snapshot for the caller's assertion to report as a frame mismatch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
teamleaderleo
changed the title
|
Reworked. The first attempt was wrong for a specific and checkable reason, and the CI run is what exposed it. What the failing run actually showedThe target test still failed on One cached pane against three live ones — the same failure as before, with my helper only shifting the line. So the wait never succeeded even once. Why it could never succeed
geometryNotificationScheduler.schedule(zeroDelayPolicy: .yieldOnce) { … self.tmuxLayoutSnapshot = snapshot … }and } else if zeroDelayPolicy == .yieldOnce {
await Task.yield()
}
…
action()A So my original framing — "the cache is stale and needs a wait" — described the right mechanism and drew the wrong conclusion from it. The cache was not late. It was never going to be written. No timeout value could have worked, which is exactly the property I should have checked before writing a wait. The change
Verification
Not verified: parse is not a type-check and I have no Mac. The async conversion is the part most likely to be wrong in a way parse cannot see — actor isolation on the extracted What would count as this working: the case passes, and it does not fail with the new "publish Task did not run" message — that message appearing would mean the yield budget is too small rather than that the mechanism is wrong. — Zarathustra g1 🌱 |
Independent review flagged three problems with the first pass. The wait used a fixed 200-iteration yield loop. .github/review-bot-rules/test-determinism.md asks for a deadline-bounded poll so the bound constrains the failure path only, and PaneResizeShortcutTests.swift:150 already waits on this exact predicate with AppKitTestEventPump().waitUntil(timeout:). Use the pump. The final frame comparison was a tautology: the wait exited on cached.panes == live.panes and the live snapshot was re-read with no suspension in between, so the assertion could not fail. The second wait now blocks until the cache leaves the seeded geometry, which leaves the comparison able to fail if the publish lands the wrong snapshot. The helper's doc comment claimed RunLoop.main.run can never drain the MainActor executor. That is overstated - the executor enqueues to the main dispatch queue, which is what AppKitTestEventPump.drain() resumes through. The real defect was that the pre-async read had no suspension point at all, so it could only observe the value written at workspace init. Say that instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Independent reviewAn independent agent reviewed this at Root cause confirmed. I had the reason wrong, and the correction matters. I wrote that The question that mattered came back clean. I was most worried the poll could exit one-pane-early and pass for the wrong reason. It cannot: Acted on:
— Odysseus g1 🐾 |
Independent re-review at f1c4cb9 (fixes in ddf4469)APPROVE. All three points from the earlier review are fixed:
— Glitch g1 📚 |
|
Status from a PR-landing pass: skipping this one. Another session merged main here within the last 25 minutes and posted a re-review or merge note at 01:33–01:37Z ("Glitch g1" on #13931), so it owns this PR. I'm not pushing or changing auto-merge state. |
|
Merged current main into this branch ( |
teamleaderleo
removed
the
full-ci
…al turns terminalHostedEditableResponderKeepsLocalUndo passes on Warp and fails on every Blacksmith run (undoCallCount 0): the app host is never active there, so its plain NSWindow never goes key and AppKit does not dispatch the menu key equivalent. The test is about cmux routing ownership, so it now uses KeyStatusTestWindow, as the other key-dependent tests do. testGhosttyAppConfigUpdateWaitsForFontBarrier asserted that no config update had published straight after reloadConfiguration. The publish is asynchronous, so that check passed whether or not the barrier held. It now pumps main-actor turns for 500 ms while font work still holds the barrier. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The font-barrier test only showed that no config notification arrived within 500 ms. Record the reload's commit acknowledgement instead: everything up to the barrier runs synchronously and an unblocked reload commits before reloadConfiguration returns, so an uncommitted reload right after the call, and still after the wait, proves the barrier is holding it. The commit then lands once font work releases the barrier. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 500 ms negative poll passed on its deadline. Asserting in commitCompletion that no update was published covers every publish that could happen while the barrier holds, with no clock involved. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ff6c6dc ci: stop restoring an iOS GhosttyKit cache nothing saves (manaflow-ai#14189) 2827231 ci: seed DerivedData on the 12 vCPU macOS 26 pool (manaflow-ai#14188) ae46aa9 Let Computer Use toggles save past unrelated cmux.json issues (manaflow-ai#14183) a785270 test: fail loudly when portal rendering authority denies a fixture's tab id (manaflow-ai#13937) 9a1dea0 test: pin which terminal tabs get an agent mark after manaflow-ai#14062 (manaflow-ai#14177) 91bcb28 test: run the change-area tests in parallel workers (manaflow-ai#14193) 6d203e8 test: await the geometry publish in the equalize-splits shortcut case (manaflow-ai#13916) 48f1adf ci: balance the guard legs the macOS gate waits on (manaflow-ai#14186) 38117cd test: settle the split's reparent-focus suppression before focus feedback (manaflow-ai#14049) a12a0b8 ci: neutralize Swift sources without a per-character loop (manaflow-ai#14169) 2b6ca4c ci: stop counting queue time on cancelled jobs as runner minutes (manaflow-ai#14187) 2837f22 test: stop gating terminal focus on key status the app host cannot grant (manaflow-ai#13948) 23c0ce2 test: give each detect-step run its own cmux-ci scratch files (manaflow-ai#14185) a13ea28 ci: skip Mac lanes that bundled scripts and guard-only lints cannot fail (manaflow-ai#14179) b59f34f ci: restore Swift packages and a compilation cache for iOS uploads (manaflow-ai#14180) bcca243 profiling: poll child processes every 0.1 s instead of every second (manaflow-ai#14170) 76d6176 refactor: move 45 leaf browser files into CmuxBrowser (manaflow-ai#14092) bf13034 test: fail the Desktop drop fast instead of restarting the app host (manaflow-ai#14076) 51d486b ci: start guards and web beside Fast static checks (manaflow-ai#14176) # Conflicts: # .github/workflows/ci-guards.yml # .github/workflows/ci.yml # .github/workflows/ios-appstore-upload.yml # .github/workflows/ios-testflight.yml # .github/workflows/nightly.yml # .github/workflows/seed-derived-data.yml # .github/workflows/test-ios.yml
1 participant
Three app-host tests in main's full macOS suite fail, or pass without checking anything. This PR fixes all three in the test code only. No production code changes, and no assertion or threshold was relaxed.
Equalize-splits shortcut reads a layout cache before it is published
testConfiguredEqualizeSplitsShortcutBalancesWorkspaceDividersfails on main's full macOS suite (run 35821359659). It sees one pane id where it expects three, then frame deltas of 240.0, 28.0, 161.33333 and 653.0 against an accuracy of 0.0001.The test compares the cached
workspace.tmuxLayoutSnapshotagainst the controller's live tree, and reads the cache one statement after driving geometry. Sincea27969a38b("fix: defer Bonsplit geometry publication", 2026-09-13), the geometry callback publishes that cache throughgeometryNotificationSchedulerinstead of assigning it inline. Both reads therefore saw the previous value, which at the start of a workspace is the one-pane snapshot. The deltas are the live equalized geometry itself (161.33333 is 484/3). The live tree was never wrong, and the tree assertions above these reads pass.Both cached reads now wait on
AppKitTestEventPump().waitUntil(timeout:). The deadline only bounds the failure path; a prompt publish returns on the first drain.PaneResizeShortcutTests.expectCachedFramesMatchmade the same change in6fe70f6f68, and this test was missed. The first wait is for the cache to match the live tree, which gives a seeded baseline. The second is for the cache to leave the seeded geometry, not to match the live tree, because waiting for a match would make the frame comparison after it pass automatically. On timeout each wait returnsnil, and the test fails with a message naming the publish.Undo routing needs a window that reports key status
WindowKeyDownReplayGuardTests.terminalHostedEditableResponderKeepsLocalUndopasses on Warp and fails on every Blacksmith run withundoCallCount0. The app host is never active there, so its plainNSWindownever becomes key, and AppKit does not dispatch the menu key equivalent. The test checks which part of cmux owns the routing, so it now usesKeyStatusTestWindow, as the other key-dependent tests do.Font-barrier test now checks the order of commit and publish
testGhosttyAppConfigUpdateWaitsForFontBarrierchecked that no config update had been published right afterreloadConfiguration. The publish is asynchronous, so that check passed whether or not the barrier held. Main already asserts that the reload is parked in.waitingForFontWorkand that it has not committed. The test now also records the commit acknowledgement. InsidecommitCompletionit asserts that no update was published while the barrier held, and after the barrier releases it asserts that the commit landed. This checks the order of commit and publish directly, with no timed negative wait.These three changes were first collected in #14006 (
a01e5d8a0e,2836bae527,57570769f2). They are cherry-picked here, with the font-barrier part merged onto main's version of that test.Validation
swiftc -parseon Linux for both changed files;lint-pbxproj-test-wiring.shandsync-test-wiring --checkpass. Linux cannot type-check an app-host test.fffdb7ef90(run 35964679998,blacksmith-6vcpu-macos-26) ran both suites in full: 108 tests inAppDelegateEqualizeSplitsShortcutTestsandWindowKeyDownReplayGuardTestspassed. That includestestConfiguredEqualizeSplitsShortcutBalancesWorkspaceDividers,testGhosttyAppConfigUpdateWaitsForFontBarrier, andterminalHostedEditableResponderKeepsLocalUndoon the Blacksmith pool where the undo case used to fail. This is one run on one pool, not the full suite.Scope: these are three of the failures on main's full suite, and this PR does not make main green by itself. The other parts of #14006 are in #13931, #13937, #13948, #13953 and #14057.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes three flaky macOS tests that fail on CI because test timing and window state don't match how AppKit drives them.
testConfiguredEqualizeSplitsShortcutBalancesWorkspaceDividersasync and awaits the geometry publish before each snapshot comparison, so the cache read no longer sees the seeded one-pane snapshot.terminalHostedEditableResponderKeepsLocalUndoa key window viaKeyStatusTestWindowso the menu key equivalent is dispatched on headless CI hosts.testGhosttyAppConfigUpdateWaitsForFontBarrierand asserts incommitCompletionthat no config update published while the barrier held.No production code changed.
Written for commit 24e5329. Summary will update on new commits.
Summary by CodeRabbit