Skip to content

Two workspace group tests expect results the code never produces - #12597

Closed
ejc3 wants to merge 1 commit into
manaflow-ai:mainfrom
ejc3:fix/workspace-group-test-expectations
Closed

ejc3 wants to merge 1 commit into
manaflow-ai:mainfrom
ejc3:fix/workspace-group-test-expectations

Conversation

@ejc3

@ejc3 ejc3 commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Two workspace group tests expect results the code has never produced.

staleGroupReferenceInsideGroupRunRendersAsRootRow checks effectiveMembership[staleMember.id] == nil. effectiveGroupIdByWorkspaceId returns [UUID: UUID?] with an entry for every tab, so the lookup is a double optional. The stale member's entry is .some(nil), which prints as nil but is not equal to nil. The test now compares against .some(nil).

mobileWorkspaceMoveGroupHeaderPreservesGroupMembership creates a group from two children and then expects the moved group to hold only the anchor and the second child. createWorkspaceGroup keeps every child, so the test now expects the anchor followed by both children at the end of the tab list.

Testing

  • Ran WorkspaceGroupTests and WorkspaceGroupMoveToMenuStateTests on an EC2 Mac (Xcode 26.6, macOS 15.7) through scripts/ci/run-app-host-xcodebuild.sh, the way CI runs app-host tests. At main 4638e5b1ea plus the compile fixes in Fix the compile errors that keep main and its unit tests from building #12584, both suites fail on the two tests above. With this change on the same base, both suites pass.

Demo Video

Not applicable. The change only affects unit tests.

Review Trigger (Copy/Paste as PR comment)

@codex review
@coderabbitai review
@greptile-apps review
@cubic-dev-ai review

Checklist

  • I tested the change locally
  • I added or updated tests for behavior changes
  • I updated docs/changelog if needed
  • I requested bot reviews after my latest commit (copy/paste block above or equivalent)
  • All code review bot comments are resolved
  • All human review comments are resolved

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 two workspace group tests that expected results the code never produces.

  • staleGroupReferenceInsideGroupRunRendersAsRootRow now checks for .some(nil) instead of nil because the lookup returns a double optional.
  • mobileWorkspaceMoveGroupHeaderPreservesGroupMembership now expects both children retained when creating a group, matching createWorkspaceGroup behavior.

Written for commit c11c251. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Tests
    • Updated workspace grouping coverage to verify that grouped workspaces retain the correct ordering when moved.
    • Added validation for trailing-tab placement after workspace group moves.
    • Improved coverage for stale workspace membership references, ensuring they are handled correctly when no active group is associated.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 2e368baf-2124-44ab-89f2-0d88b3abba7f

📥 Commits

Reviewing files that changed from the base of the PR and between 211b8bb and c11c251.

📒 Files selected for processing (2)
  • cmuxTests/WorkspaceGroupMoveToMenuStateTests.swift
  • cmuxTests/WorkspaceGroupTests.swift

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

Two workspace grouping tests now match the current grouped-workspace ordering and double-optional membership lookup behavior.

Changes

Workspace grouping tests

Layer / File(s) Summary
Grouping and membership assertions
cmuxTests/WorkspaceGroupMoveToMenuStateTests.swift, cmuxTests/WorkspaceGroupTests.swift
The grouped-workspace expectations now include the second original workspace. The stale-member assertion now expects .some(nil) for an existing membership entry with no group.

Priority: ⬇️ Low

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

Change: Other

Suggested reviewers: austinywang

Merge Risk: ⚪ Minimal · up to c11c2

This test-only change introduces no identified production behavior or merge-blocking risk.

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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 3 functions across 2 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 summarizes the main change: two workspace group tests had expectations that did not match the implementation.
Description check ✅ Passed The description includes the required Summary, Testing, Demo Video, Review Trigger, and Checklist sections. It explains both test corrections and documents the test suites and environment used.
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 Swift Actor Isolation ✅ Passed PASS. The authoritative diff changes only two files under cmuxTests/, with five assertion/comment edits and no production Swift files. The modified code remains inside existing @MainActor test str…
Cmux Swift Blocking Runtime ✅ Passed PASS: The pull request changes only cmuxTests/WorkspaceGroupMoveToMenuStateTests.swift and cmuxTests/WorkspaceGroupTests.swift. The diff only updates test expectations and comments. It adds no blo…
Cmux Browser Automation Off-Main ✅ Passed PASS. The pull request changes only two workspace-group test files. The diff updates expected tab ordering and a double-optional assertion. It does not change browser socket commands, WebKit/AppKit ac…
Cmux Expensive Synchronous Load ✅ Passed PASS: The authoritative PR diff changes only cmuxTests/WorkspaceGroupMoveToMenuStateTests.swift and cmuxTests/WorkspaceGroupTests.swift (5 insertions, 2 deletions). The patch updates test expectat…
Cmux Cache Substitution Correctness ✅ Passed PASS. The pull request changes only two files under cmuxTests/: WorkspaceGroupMoveToMenuStateTests.swift and WorkspaceGroupTests.swift. The diff updates test expectations only. It does not chang…
Cmux No Hacky Sleeps ✅ Passed PASS. The authoritative diff changes only two Swift test files: cmuxTests/WorkspaceGroupMoveToMenuStateTests.swift and cmuxTests/WorkspaceGroupTests.swift. The changes only update assertions and c…
Cmux Algorithmic Complexity ✅ Passed PASS. The review-scoped diff changes only two files under cmuxTests/. The five added and two removed lines update test expectations and comments only; no production Swift, TypeScript, JavaScript, sh…
Cmux Swift Concurrency ✅ Passed PASS: The scoped diff changes only two XCTest expectations and related comments in cmuxTests/WorkspaceGroupMoveToMenuStateTests.swift and cmuxTests/WorkspaceGroupTests.swift. It adds no `DispatchQ…
Cmux Swift @Concurrent ✅ Passed PASS. The reviewed diff changes only test expectations and comments in two Swift test files. It adds no async function, call site, or concurrency annotation. The existing @MainActor test isolation is …
Cmux Swift Package Boundaries ✅ Passed PASS. The pull request changes only two files under cmuxTests/, with five insertions and two deletions. The changes update test expectations and comments; they do not introduce or expand production …
Cmux Swiftpm Lockfiles ✅ Passed PASS. The authoritative PR diff changes only two files under cmuxTests/: test assertions and comments. It contains no Package.swift, Package.resolved, .gitignore, Xcode project, workflow, or d…
Cmux Swift Logging ✅ Passed The pull request changes only two Swift test files. The diff updates assertions and comments; it adds no print, debugPrint, dump, NSLog, file/stdout logging, Logger declaration, or sensitive-data logg…
Cmux User-Facing Error Privacy ✅ Passed PASS. The authoritative diff changes only two files under cmuxTests/. The changes update test expectations and comments; they do not change production behavior or user-facing errors, alerts, command…
Cmux Full Internationalization ✅ Passed PASS. The scoped diff changes only two files under cmuxTests/: two test expectations and a test comment. The internationalization rule explicitly allows tests and developer-only comments. The diff a…
Cmux Swiftui State Layout ✅ Passed PASS: The pull request changes only two test expectation sites in cmuxTests/WorkspaceGroupMoveToMenuStateTests.swift and cmuxTests/WorkspaceGroupTests.swift. The diff adds no ObservableObject, p…
Cmux Architecture Rethink ✅ Passed PASS. The pull request changes only two test files and adds no architectural behavior. The diff only updates assertions and comments: it expects .some(nil) for the existing [UUID: UUID?] membershi…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS. The pull request changes only two test files: cmuxTests/WorkspaceGroupMoveToMenuStateTests.swift and cmuxTests/WorkspaceGroupTests.swift. The diff changes test assertions and comments only. …
Cmux Source Artifacts ✅ Passed The pull request changes only two hand-written Swift test files: cmuxTests/WorkspaceGroupMoveToMenuStateTests.swift and cmuxTests/WorkspaceGroupTests.swift. The diff updates test assertions and co…
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS: The authoritative PR diff contains only cmuxTests/WorkspaceGroupMoveToMenuStateTests.swift and cmuxTests/WorkspaceGroupTests.swift. It contains no Swift file under a production Sources/ pa…
Cmux No Ambient Global State ✅ Passed The pull request changes only two test files: cmuxTests/WorkspaceGroupMoveToMenuStateTests.swift and cmuxTests/WorkspaceGroupTests.swift. The diff changes test expectations and adds comments; it i…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@github-actions

Copy link
Copy Markdown
Contributor

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

@ejc3
ejc3 marked this pull request as ready for review September 14, 2026 18:20
`staleGroupReferenceInsideGroupRunRendersAsRootRow` checks
`effectiveMembership[staleMember.id] == nil`. `effectiveGroupIdByWorkspaceId`
returns `[UUID: UUID?]` with an entry for every tab, so the lookup is a double
optional and the stale member's `.some(nil)` never equals `nil`. Compare
against `.some(nil)`.

`mobileWorkspaceMoveGroupHeaderPreservesGroupMembership` creates a group from
two children, but expects the moved group to contain only the anchor and the
second child. Group creation keeps every child, so expect the anchor followed
by both children at the end of the tab list.
@ejc3
ejc3 force-pushed the fix/workspace-group-test-expectations branch from e4b5ad9 to c11c251 Compare September 14, 2026 20:45
@cursor

cursor Bot commented Sep 14, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

ejc3 added a commit to ejc3/cmux that referenced this pull request Sep 14, 2026
ejc3 added a commit to ejc3/cmux that referenced this pull request Sep 19, 2026
ejc3 added a commit to ejc3/cmux that referenced this pull request Sep 19, 2026
ejc3 added a commit to ejc3/cmux that referenced this pull request Sep 19, 2026
ejc3 added a commit to ejc3/cmux that referenced this pull request Sep 20, 2026
ejc3 added a commit to ejc3/cmux that referenced this pull request Sep 20, 2026
ejc3 added a commit to ejc3/cmux that referenced this pull request Sep 20, 2026
@teamleaderleo

Copy link
Copy Markdown
Collaborator

Thanks for tracking this one down! Main picked up the same fix in c8bfb58, so I'm closing this as done. Really appreciate all the test cleanup :)

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.

2 participants