Repository navigation
Add split cycle terminal rendering regression test - #3789
lawrencecchen wants to merge 1 commit into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request adds a new regression test that verifies terminal surface stability during repeated split/unsplit operations. The test launches the app, repeatedly creates and removes terminal splits via keyboard shortcuts, and verifies that the original leftmost terminal surface remains alive, maintains its identity, and continues to re-render after each cycle. ChangesTerminal Surface Render Verification Regression
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ 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 adds an XCUITest (
Confidence Score: 3/5The test-only change is safe to merge but has a correctness gap in its own assertion logic that could hide the very rendering regression it was written to catch. The silent discard of cmuxUITests/BrowserPaneNavigationKeybindUITests.swift — specifically Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as XCUITest
participant App as cmux App
participant Socket as Control Socket
participant Terminal as Ghostty Terminal
Test->>App: "launch (CMUX_UI_TEST_MODE=1)"
Test->>Socket: ping → PONG
Test->>Socket: surface.list → initialSurfaceId
Test->>Socket: debug.terminal.render_stats (baseline)
Test->>Socket: debug.panel_snapshot.reset
Test->>Socket: debug.panel_snapshot (baseline snapshot)
Test->>Socket: surface.send_text (marker command)
Test->>Socket: surface.read_text (wait for marker text)
Test->>Socket: debug.terminal.render_stats (wait counter advance)
Test->>Socket: debug.panel_snapshot (after snapshot)
Note over Test: Assert changedPixels > 20
loop 10 cycles
Test->>App: Cmd+D (split)
Test->>Socket: "surface.list (wait count=2)"
Test->>App: Cmd+D (split)
Test->>Socket: "surface.list (wait count=3)"
Test->>App: Ctrl+D (close)
Test->>Socket: "surface.list (wait count=2)"
Test->>App: Ctrl+D (close)
Test->>Socket: "surface.list (wait count=1)"
Test->>Socket: "surface.list → remaining[0].id == initialSurfaceId"
Test->>Terminal: assertSurfaceRendersNewMarker(cycle N)
end
Reviews (1): Last reviewed commit: "Add split cycle terminal rendering regre..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "\(context): expected left terminal window to be visible. stats=\(baselineStats)" | ||
| ) | ||
|
|
||
| _ = socketResult(method: "debug.panel_snapshot.reset", params: ["surface_id": surfaceId]) |
There was a problem hiding this comment.
changedPixels across cycles
debug.panel_snapshot.reset discards its result, so a socket error or a missing implementation won't surface during the test. If the reset silently fails, the snapshot accumulator is never zeroed: every subsequent debug.panel_snapshot call for that surface will report changedPixels that include accumulated changes from all previous cycles. In cycle 2 onward, the changedPixels > 20 assertion can pass even if the terminal renders nothing new for the current marker, giving false confidence that the regression is caught.
| } | ||
| return stats.presentCount > baselineStats.presentCount | ||
| || stats.drawCount > baselineStats.drawCount |
There was a problem hiding this comment.
presentCount and drawCount weakens the render-advance assertion
The condition fires as soon as either counter moves for any reason (cursor blink, focus change, window resize) — it does not require both the draw and the present to have happened for this specific frame. Since waitForSurfaceText has already confirmed the text is in the terminal model before this block runs, the render counters will almost always be satisfied immediately on the first poll iteration, making the 6-second guard near-vacuous. Using AND (&&) would be a stricter signal that a full draw→present cycle completed for the marked frame.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-splitrender build-for-testing -only-testing:cmuxUITests/BrowserPaneNavigationKeybindUITests/testRepeatedCmdDSplitAndCtrlDExitKeepsLeftmostTerminalRenderingpassed.\n-test-e2e.ymlpassed forBrowserPaneNavigationKeybindUITests/testRepeatedCmdDSplitAndCtrlDExitKeepsLeftmostTerminalRendering: https://github.com/manaflow-ai/cmux/actions/runs/25593148882\n\n## Task\n- Regression coverage for repeated split and terminal exit churn making the leftmost Ghostty terminal fail to render.