Skip to content

Report the closed surface's own ref from surface.close - #14698

Merged
teamleaderleo merged 3 commits into
mainfrom
close-surface-wrong-target
Sep 25, 2026
Merged

teamleaderleo merged 3 commits into
mainfrom
close-surface-wrong-target

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

cmux close-surface --surface surface:N answers OK surface:<N+1-ish> instead of OK surface:N. The new ref points at the surface that was just closed, so it never shows up anywhere and looks like a transient replacement panel. That is one of the two symptoms in #12524.

Cause: closing a tab runs cleanupSurfaceState, which removes the surface's ref from the handle registry. surfaceClose then built the reply with ref(.surface, id), which mints a brand new ref for the dead UUID (and leaves that entry in the registry for good).

Fix: look up the target's existing ref (new non-minting ControlHandleRegistry.existingRef) before closing and use it in the reply. If the closed surface differs from the requested one, or no ref existed, the reply keeps the old behavior.

This does not explain the other, more serious symptom in #12524 (the wrong surface being closed), so the issue stays open.

Tests: ControlCommandCoordinatorSurfaceTests/surfaceClosePayloadReportsTheClosedSurfaceRef (first commit fails without the fix: the reply carries surface:3; second commit is the fix).

Refs #12524

🤖 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 surface.close replying with a freshly minted ref (OK surface:<N+1>) instead of the closed surface's own ref.

Written for commit 71ef081. Summary will update on new commits.

Review in cubic

teamleaderleo and others added 2 commits September 25, 2026 15:46
Closing a surface forgets its ref during teardown, so building the
reply afterwards mints a fresh ref for the dead surface (#12524 saw
OK surface:<new id>). This test fails until the reply uses the ref
captured before the close.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Teardown removes a closed surface's ref from the handle registry before
the reply is built, so the reply minted a new ref (OK surface:<N+1>)
for a surface that no longer exists. Look up the target's existing ref
before closing and use it in the reply.

Refs #12524

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

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 37 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ca7235d5-70cc-4fed-bcb9-983574d72c00

📥 Commits

Reviewing files that changed from the base of the PR and between a75ab64 and 71ef081.

📒 Files selected for processing (4)
  • Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator+Surface.swift
  • Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlHandleRegistry.swift
  • Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorSurfaceTests.swift
  • Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeSurfaceControlCommandContext.swift

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.

@github-actions

Copy link
Copy Markdown
Contributor

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

…tab_id

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@teamleaderleo
teamleaderleo merged commit 67debd6 into main Sep 25, 2026
10 of 13 checks passed
@teamleaderleo
teamleaderleo deleted the close-surface-wrong-target branch September 25, 2026 19:51
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for 71ef081f3f, merged 2026-09-25 19:51:30 UTC

  • Not verified at merge: ci-status (not reported), Web complexity (in progress), web-validation (not reported)
  • Skipped by policy: web-build, web-tests
  • Full suite: runs on main after merge.

Labeled merged-unverified: if main breaks near this merge, look here first.

@github-actions github-actions Bot added the merged-unverified A judging check was not green at merge; see the merge receipt comment label Sep 25, 2026
rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 25, 2026
8538fa9 Add a Focus Last action that toggles between the two most recent focus positions (manaflow-ai#14700)
fba6c47 Don't leak the host's TERM_PROGRAM/COLORTERM into remote PTY sessions (manaflow-ai#9610)
10cdafe agent-chat: skip the launchd PATH prefix on Windows so agent CLIs resolve (manaflow-ai#12206)
9c8039a Add Reveal in Finder to the terminal context menu (manaflow-ai#14697)
d57f584 Remove dead resume code left by manaflow-ai#14560 (manaflow-ai#14693)
67debd6 Report the closed surface's own ref from surface.close (manaflow-ai#14698)
a75ab64 test(ios): fix stale CmuxMobileShell connection-recovery tests (manaflow-ai#14691)
ec7b2f1 Predicted echo: seed alternate screen from ghostty, guard stale erases (manaflow-ai#14686)
9aa850d refactor: move the Cloud surface models into CmuxCloud (manaflow-ai#14390)
da468e1 ci(e2e): order sibling waits by attempt start, adopt main's seed product (manaflow-ai#14684)
ff02854 Request badge authorization so the Dock badge renders (manaflow-ai#14242)
dc89b82 ci: iOS picker mints the routing token with the org runner permission (manaflow-ai#14690)
9650672 ci: label the org glaeda-minis runners with warm keys (manaflow-ai#14679)
992a2de Remove unreachable persistent-SSH resume binding code (manaflow-ai#14560)
6b4d976 ci(ios): read idle simulator minis live from the runners API (manaflow-ai#14542)
2f5d439 fix(ios): align hidden-marker and Iroh aggregation tests with build identity (manaflow-ai#14535)
bcf0122 Start a never-shown terminal before surface.read_text and read_screen (manaflow-ai#14673)
60469a3 ci: reuse unit xctestrun for numeric locale tests (manaflow-ai#13414)
2d1bf1b ci: give the receipt contract's guard fixture every workflow (manaflow-ai#14685)
ec52ce4 test: give the restore surface-context test its own socket (manaflow-ai#14680)

# Conflicts:
#	.github/workflows/ci-cache-receipts.yml
#	.github/workflows/ci-owned-warm-labels.yml
#	.github/workflows/seed-derived-data.yml
#	.github/workflows/test-e2e.yml
#	.github/workflows/test-ios.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-unverified A judging check was not green at merge; see the merge receipt comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant