Skip to content

test: a window with no restorable workspaces is dropped from the snapshot - #14801

Merged
teamleaderleo merged 1 commit into
mainfrom
fix/phantom-window-test-expectation
Sep 26, 2026
Merged

teamleaderleo merged 1 commit into
mainfrom
fix/phantom-window-test-expectation

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

TabManagerChildExitCloseTests.testSessionSnapshotKeepsWindowWithNoRestorableWorkspaces fails on current main, at TabManagerUnitTests.swift:947 ("XCTUnwrap failed: expected non-nil value of type AppSessionSnapshot"). It first showed up on #14789 after main was merged into it (run 36218677612, shard 5/7).

Root cause

#14788 made SessionPersistencePolicy.pruningCmuxCrashDiagnosticWindows drop phantom windows, meaning windows with no workspaces and no window Dock, on every save. The test builds a window whose only workspace is a non-restorable remote one. That window is now a phantom, so the snapshot is nil. #14788 updated AppDelegateDisplayConfigRestoreTests for this but missed this suite, because it wasn't among #14788's changed suites.

Change

Rename the test to testSessionSnapshotDropsWindowWithNoRestorableWorkspaces and assert the new policy: the snapshot is nil. Test-only.

Test plan

  • CI app-host: TabManagerChildExitCloseTests green

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes the failing TabManagerChildExitCloseTests test that still expected buildSessionSnapshot to keep a window with no restorable workspaces. The session policy now drops such windows as phantom windows, so the test asserts the snapshot is nil instead.

  • Renames the test to testSessionSnapshotDropsWindowWithNoRestorableWorkspaces and updates its assertion to match the phantom-window policy.

Written for commit 288570a. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Tests
    • Updated session snapshot coverage to confirm windows with no restorable workspaces are excluded.

…shot

#14788 made the session policy drop phantom windows (no workspaces, no
window Dock) on every save. TabManagerChildExitCloseTests still expected
buildSessionSnapshot to keep a window whose only workspace is a
non-restorable remote one, so it failed with a nil snapshot on every run
that selects it (first seen on #14789 after merging main). Assert the new
policy instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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 configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: dbe82e6a-db10-43c3-b11e-fef75043deac

📥 Commits

Reviewing files that changed from the base of the PR and between fb665a0 and 288570a.

📒 Files selected for processing (1)
  • cmuxTests/TabManagerUnitTests.swift

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The test for a window with no restorable workspaces now expects sessionSnapshotForTesting() to return nil.

Changes

Session snapshot policy

Layer / File(s) Summary
Update snapshot test expectation
cmuxTests/TabManagerUnitTests.swift
The test is renamed to cover dropping a window with no restorable workspaces. It now expects a nil session snapshot.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to 28857

This test-only change aligns the expectation with the existing session-save policy and presents no material merge risk.

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (24 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the test change: windows with no restorable workspaces are dropped from the snapshot.
Description check ✅ Passed The description explains the failing test, root cause, expected behavior, and test-only scope. It identifies the intended CI test, although it does not report completed test execution and omits the te…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Cloud Persistent Session And Early Input ✅ Passed The pull request changes only cmuxTests/TabManagerUnitTests.swift. It renames a session snapshot test and changes its assertion from an unwrapped snapshot to XCTAssertNil. It does not change Cloud…
Cmux Swift Actor Isolation ✅ Passed PASS: The review-scoped diff changes only cmuxTests/TabManagerUnitTests.swift. It renames a test and changes its assertion from an unwrapped snapshot to XCTAssertNil; it adds no production Swift c…
Cmux Swift Blocking Runtime ✅ Passed The pull request changes only cmuxTests/TabManagerUnitTests.swift. It renames a test, adds a comment, and changes the assertion to XCTAssertNil; it introduces no production Swift code or blocking/…
Cmux Browser Automation Off-Main ✅ Passed The pull request changes only cmuxTests/TabManagerUnitTests.swift. The diff renames one test and changes its snapshot assertion to XCTAssertNil. It does not change browser socket commands, `proces…
Cmux Expensive Synchronous Load ✅ Passed PASS. The pull request changes only cmuxTests/TabManagerUnitTests.swift. It renames a test and changes the assertion from an unwrapped snapshot to XCTAssertNil; it adds no production Swift code or…
Cmux Cache Substitution Correctness ✅ Passed PASS: The pull request changes only cmuxTests/TabManagerUnitTests.swift. It renames a test and changes the assertion from an unwrapped snapshot to XCTAssertNil; it does not replace any production …
Cmux No Hacky Sleeps ✅ Passed PASS. The pull request changes only cmuxTests/TabManagerUnitTests.swift. It updates a Swift test name, comment, and assertion. It introduces no TypeScript, JavaScript, shell, build/runtime, or produ…
Cmux Algorithmic Complexity ✅ Passed PASS. The PR changes only cmuxTests/TabManagerUnitTests.swift. It renames a test, adds a comment, and changes assertions from snapshot contents to XCTAssertNil; it adds no production algorithm or …
Cmux Swift Concurrency ✅ Passed PASS: The PR changes only a Swift test name, a documentation comment, and the snapshot assertion. It adds no Dispatch, Combine, completion-handler, or fire-and-forget Task pattern. Existing async patt…
Cmux Swift @Concurrent ✅ Passed PASS: The PR changes only cmuxTests/TabManagerUnitTests.swift. The changed test remains synchronous (throws only), and the diff adds no async, nonisolated async, or @concurrent code. It only…
Cmux Swift Package Boundaries ✅ Passed PASS: The PR changes only cmuxTests/TabManagerUnitTests.swift. It renames a test, updates its assertion, and adds a test comment. It introduces no production Swift code or app-target feature logic. …
Cmux Swiftpm Lockfiles ✅ Passed PASS. The pull request changes only cmuxTests/TabManagerUnitTests.swift. It does not change a SwiftPM package, Package.resolved, an Xcode project package reference, .gitignore, a workflow, or a …
Cmux Swift Logging ✅ Passed The pull request changes only a Swift test in cmuxTests/TabManagerUnitTests.swift. The diff renames the test, adds a comment, and changes assertions from an expected snapshot to XCTAssertNil; it a…
Cmux User-Facing Error Privacy ✅ Passed PASS: The pull request changes only cmuxTests/TabManagerUnitTests.swift. It renames a test, updates the assertion to expect nil, and adds a developer-only test comment. No production user-facing e…
Cmux Full Internationalization ✅ Passed PASS: The authoritative diff changes only cmuxTests/TabManagerUnitTests.swift. It renames a test, updates its assertion from an unwrapped snapshot to XCTAssertNil, and adds a test comment. No prod…
Cmux Swiftui State Layout ✅ Passed The pull request changes only a test in cmuxTests/TabManagerUnitTests.swift. It renames the test and changes the snapshot assertion to XCTAssertNil; it does not add or modify SwiftUI state, layout…
Cmux Architecture Rethink ✅ Passed PASS: The PR changes only one XCTest. It renames the test, adds an invariant comment, and changes the expectation to nil for a non-restorable-only window. The diff adds no timing repair, mutable state…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The PR changes only cmuxTests/TabManagerUnitTests.swift. It renames a test and changes its assertion from an unwrapped snapshot to XCTAssertNil; it adds no NSWindow, NSPanel, `NSWindowControll…
Cmux Source Artifacts ✅ Passed PASS: The PR changes only the tracked hand-written test source cmuxTests/TabManagerUnitTests.swift. The 4-line change updates a test name, adds an explanatory comment, and changes the assertion. It …
Cmux No Test Or Debug Seam In Production Source ✅ Passed The pull request changes only cmuxTests/TabManagerUnitTests.swift, which is test code. The diff adds no Swift file under a production Sources/ path and adds no production test or debug seam.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@teamleaderleo
teamleaderleo merged commit cc90659 into main Sep 26, 2026
104 of 113 checks passed
@teamleaderleo
teamleaderleo deleted the fix/phantom-window-test-expectation branch September 26, 2026 06:05
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for 288570a4ab: every check was green at merge (16 verified; 14 skipped by policy). Full suite runs on main after merge.

rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 26, 2026
cc90659 test: a window with no restorable workspaces is dropped from the snapshot (manaflow-ai#14801)
9b10f7c Merge pull request manaflow-ai#14756 from manaflow-ai/12956-ssh-auth-followup-main
fb665a0 test(ime): install option-as-alt right before the dead-key dispatch (manaflow-ai#14800)
9d459e3 fix(fork): an access-time update no longer discards a fresh fork validation (manaflow-ai#14799)
39e2c3a fix(ssh): share one route check across concurrent cmux ssh opens
c344ce9 test: cover concurrent cmux ssh opens sharing one route check
9ff9017 fix(ssh): report OpenSSH failures from cmux ssh instead of a Cloud VM error
311797d test: cover cmux ssh failing fast on refused and unreachable hosts
bn-l pushed a commit to bn-l/cmux that referenced this pull request Oct 4, 2026
… the snapshot

Ported from upstream manaflow-ai#14801 (cc90659): "test: a window with no restorable workspaces is dropped from the snapshot (manaflow-ai#14801)".

Companion to the manaflow-ai#14788 port: the phantom-window policy now drops a window whose
only workspace is a non-restorable remote one, so the fork's test asserting the
old keep-empty-window behavior failed with a nil snapshot. The fork has no
window Dock or member-less pinned groups, so upstream's extra keep conditions
do not apply.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant