Repository navigation
Restore Ghostty overlay CI coverage - #4913
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughCI re-enables ChangesGhosttySurfaceOverlayTests Stability and Re-enablement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 17 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (17 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 |
Greptile SummaryThis PR deflakes
Confidence Score: 4/5Changes are entirely test-scoped; production behavior is unchanged. The teardown lifecycle and focus-test AppDelegate wiring are careful but have a subtle ordering question worth checking before merging. The testPreferredScrollerStyleChangeRestoresOverlayScrollbarWidth behavioral assertion was dropped in favour of a precondition check, quietly reducing what the test actually verifies. In testSearchOverlayFocusesSearchFieldAfterDeferredAttach, the surface is both appended to surfacesToRelease and indirectly owned by the tab manager that is restored before tearDown() runs, creating a potential double-release ordering question. Neither issue affects production code, but they could let regressions slip through undetected or introduce new test instability. cmuxTests/TerminalAndGhosttyTests.swift — the weakened scrollbar assertion and the surface-lifecycle ordering in the focus test both warrant a second look. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Test starts] --> B[makeTrackedTerminalSurface]
B --> C[surface added to surfacesToRelease]
C --> D[Test body runs]
D --> E{Special case: focus test?}
E -- yes --> F[Register TabManager + AppDelegate.shared]
F --> G[Obtain surface from terminalPanel\nappend to surfacesToRelease]
G --> H[Test assertions / waitUntil loop]
H --> I[defer: restore TabManager + AppDelegate.shared]
I --> J[tearDown runs]
E -- no --> H
H --> J
J --> K[releaseSurfaceForTesting for each in surfacesToRelease reversed]
K --> L[surfacesToRelease cleared]
L --> M[super.tearDown]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| XCTAssertEqual(scrollView.scrollerStyle, .legacy) | ||
| assertPendingSurfaceWidth( | ||
| initialSurfaceSize.width, | ||
| "Changing the scroll view style alone should leave the terminal grid stale until the scroller-style observer runs" | ||
| "Changing the scroll view style alone should leave the terminal grid unchanged until the scroller-style observer runs" | ||
| ) |
There was a problem hiding this comment.
XCTAssertEqual(scrollView.scrollerStyle, .legacy) only confirms that the assignment took effect, not that the terminal content area was actually affected. If the scrollerStyle-to-content-width mapping regresses, this assertion won't catch it. The assertion below (restoredContentWidth >= legacyContentWidth) covers the recovery direction but not the initial contraction. If the flake was caused by the assertion failing on headless CI runners that report zero-size scroll views, a better deflake guard would be to skip the assertion when initialContentWidth == 0 rather than dropping the check entirely.
| XCTAssertEqual(scrollView.scrollerStyle, .legacy) | |
| assertPendingSurfaceWidth( | |
| initialSurfaceSize.width, | |
| "Changing the scroll view style alone should leave the terminal grid stale until the scroller-style observer runs" | |
| "Changing the scroll view style alone should leave the terminal grid unchanged until the scroller-style observer runs" | |
| ) | |
| XCTAssertEqual(scrollView.scrollerStyle, .legacy) | |
| if initialContentWidth > 0 { | |
| XCTAssertLessThan( | |
| legacyContentWidth, | |
| initialContentWidth, | |
| "Legacy scrollbars should reserve width in the scroll view content area" | |
| ) | |
| } | |
| assertPendingSurfaceWidth( | |
| initialSurfaceSize.width, | |
| "Changing the scroll view style alone should leave the terminal grid unchanged until the scroller-style observer runs" | |
| ) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This branch was successfully deployed
1 participant
Summary
GhosttySurfaceOverlayTestscleanup and search overlay focus assertionsGhosttySurfaceOverlayTestsFixes #4524.
Verification
git diff --checkxcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-ghostty-overlay-build CMUX_SKIP_ZIG_BUILD=1 build -quiet\n- AWS:cmuxTests/GhosttySurfaceOverlayTestspassed, result bundle/tmp/cmux-ghostov-fix3-1779927163.xcresultNeed help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
Changes are limited to XCTest helpers and CI test selection; no production terminal or overlay behavior is modified.
Overview
Re-enables
GhosttySurfaceOverlayTestsin CI by dropping thexcodebuild -skip-testingentry for that class in the unit-test job.The overlay test suite is hardened for flaky app-host runs:
tearDownclears debug key observers and releases trackedTerminalSurfaceinstances viamakeTrackedTerminalSurface,waitUntilpolls on the main run loop instead ofXCTNSPredicateExpectation, and find-overlay focus tests use a fake key window plusAppDelegate/TabManagertest registration. Scroller-style expectations are loosened so legacy vs overlay width checks are less brittle on runners, and the retention test explicitly releases the surface before asserting deallocation.Reviewed by Cursor Bugbot for commit c2d15b1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Restores CI coverage for Ghostty overlay by deflaking
GhosttySurfaceOverlayTestsand removing the CI skip. Fixes #4524.KeyStatusTestWindowto stabilize search overlay focus and mount timing.TerminalSurfaceinstances intearDownto prevent leaks; added a retain-cycle check.Written for commit c2d15b1. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
Tests