Skip to content

Preserve iPhone viewport lease across output stream churn - #13548

Merged
teamleaderleo merged 6 commits into
mainfrom
fix/13474-viewport-lease-followup
Sep 22, 2026
Merged

teamleaderleo merged 6 commits into
mainfrom
fix/13474-viewport-lease-followup

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to #13498 / #13474.

The focused TerminalSurfaceMountOwnershipTests run exposed one remaining ownership mismatch after #13498: MobileShellComposite.unregisterTerminalOutput still cleared the viewport whenever the output stream ended. A transient UIKit detach therefore advanced the viewport generation and dropped the cached report even though the presentation still owned the terminal.

This patch separates those lifetimes:

  • ownerless and release-gate output streams keep the existing clear-on-termination behavior;
  • the mounted UI stream unregisters output delivery while preserving the presentation-owned viewport lease;
  • explicit presentation teardown remains the generation-fenced viewport release point.

The existing mount ownership regression from #13498 now exercises the intended rule directly.

Verification

  • Direct macOS/iPhone Simulator run: CmuxMobileShellUITests/TerminalSurfaceMountOwnershipTests — 3/3 tests passed in 15.204s, TEST SUCCEEDED.
  • The previously failing transient-detach test now passes while the same suite also proves explicit presentation teardown releases the viewport lease.
  • Temporary verification workflows were removed; the PR diff remains production code only.

Summary by cubic

Fixes the iPhone terminal viewport being dropped when the output stream ends during a transient UIKit detach, losing the cached report while the presentation still owns the terminal.

  • MobileShellComposite.terminalOutputStream now accepts a releaseViewportOnTermination flag; the mounted UI stream passes false so stream churn from UIKit detaches no longer clears the viewport.
  • Ownerless and release-gate streams keep the existing clear-on-termination behavior, and explicit presentation teardown remains the generation-fenced release point.

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

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Preserved terminal viewport state during temporary UI detachments and consumer restarts.
    • Prevented terminal output stream termination from unnecessarily clearing the active viewport.
    • Ensured viewport ownership is released explicitly during final teardown while retaining previous behavior for standard streams.

@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 22, 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: 7bc190fb-792f-4aec-ab67-87ef4c85c8a6

📥 Commits

Reviewing files that changed from the base of the PR and between 2c4282e and ebe3d6f.

📒 Files selected for processing (2)
  • Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift
  • Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift

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


📝 Walkthrough

Walkthrough

The terminal output stream now separates stream cleanup from viewport lease cleanup. Mounted UIKit surfaces keep the viewport lease across stream termination and release it during explicit teardown. The default stream API retains viewport release behavior.

Changes

Viewport Lease Lifecycle

Layer / File(s) Summary
Configurable stream cleanup
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift
unregisterTerminalOutput now accepts a release flag. The stream factory forwards this flag and clears the viewport only when enabled.
Presentation-owned viewport release
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift
Mounted surfaces request stream termination without releasing the viewport lease.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant GhosttySurfaceRepresentable
  participant CmuxMobileShell
  participant StreamTeardown
  participant Viewport
  GhosttySurfaceRepresentable->>CmuxMobileShell: request output stream with releaseViewportOnTermination false
  CmuxMobileShell->>StreamTeardown: retain release flag
  StreamTeardown->>CmuxMobileShell: unregister output with releaseViewport false
  CmuxMobileShell->>Viewport: preserve viewport lease
  GhosttySurfaceRepresentable->>Viewport: release lease during explicit teardown
Loading

Merge Risk: ⚪ Minimal · up to ebe3d

The change preserves the viewport lease across transient stream detaches and releases it during presentation teardown; no actionable merge-blocking risk remains.

🚥 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 1 functions across 1 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (24 passed)
Check name Status Explanation
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 PASS. The authoritative diff changes only iOS output-stream and viewport-lease lifetime handling in two files. It does not change Cloud terminal creation, cmux-tui clients, physical transports, manual…
Cmux Swift Actor Isolation ✅ Passed PASS. The production diff only adds a Bool parameter and conditional cleanup inside the existing @MainActor MobileShellComposite, plus passes that value from the existing UI coordinator task. `G…
Cmux Swift Blocking Runtime ✅ Passed PASS. The PR changes two production Swift files, but the authoritative diff adds no semaphore waits, sleeps, delayed dispatch, polling, main-queue sync, or manual locks. It only adds a Boolean lease f…
Cmux Browser Automation Off-Main ✅ Passed PASS. The pull request changes only iOS terminal viewport/output-stream lifecycle code in two files. The authoritative diff adds no browser socket command, WebKit wait, worker-router change, main-acto…
Cmux Expensive Synchronous Load ✅ Passed PASS. The authoritative diff changes only terminal output-stream teardown semantics in two Swift files. It adds no agent-history loader, file scan, JSON/JSONL parsing, transcript or trajectory read, o…
Cmux Cache Substitution Correctness ✅ Passed The diff changes terminal output teardown and viewport lease ownership only. It does not replace a fresh authoritative read with a cached or opportunistic value, and it does not alter persistence, his…
Cmux No Hacky Sleeps ✅ Passed PASS. The pull request changes only Swift files: MobileShellComposite.swift and GhosttySurfaceRepresentable.swift. The custom check applies to TypeScript, JavaScript, shell, and non-Swift build/ru…
Cmux Algorithmic Complexity ✅ Passed The pull request does not introduce an algorithmic-complexity violation. The Swift changes add a Boolean parameter, conditional viewport release, and an overload delegation. The existing output loop r…
Cmux Swift Concurrency ✅ Passed PASS. The diff changes viewport-release state and adds an overload; it does not introduce a new legacy async pattern. The existing Task { @mainactor in ... } termination hop and stored output task r…
Cmux Swift @Concurrent ✅ Passed The PR introduces no @concurrent or nonisolated async code. The new terminalOutputStream(...releaseViewportOnTermination:) overload is synchronous and belongs to @MainActor `MobileShellComposi…
Cmux Swift Package Boundaries ✅ Passed The diff changes only existing SwiftPM targets: CmuxMobileShell and CmuxMobileShellUI. It adds viewport-release semantics to an existing terminal stream and passes that option from `GhosttySurface…
Cmux Swiftpm Lockfiles ✅ Passed The pull request changes only two Swift source files. It does not change Package.swift, Package.resolved, .gitignore, workflow files, or Xcode project package references. Therefore, no SwiftPM lockfil…
Cmux Swift Logging ✅ Passed The PR diff changes stream and viewport-lifetime handling only. It adds no print, debugPrint, dump, NSLog, ad hoc file/stdout logging, or sensitive-data logging. The existing mobileShellLog …
Cmux User-Facing Error Privacy ✅ Passed The authoritative diff changes only terminal stream teardown behavior and internal Swift comments. It adds releaseViewport handling and passes releaseViewportOnTermination: false from the mounted …
Cmux Full Internationalization ✅ Passed PASS: The pull request changes only Swift stream and viewport-lease lifecycle behavior. Added and modified text is limited to developer comments and API documentation; no user-facing Swift text, local…
Cmux Swiftui State Layout ✅ Passed The PR changes only terminal stream teardown and viewport-lease handling. The added Swift code contains no new ObservableObject, @Published, @StateObject, @EnvironmentObject, GeometryReader,…
Cmux Architecture Rethink ✅ Passed The PR is a small correctness fix with a clear invariant. The diff adds no sleeps, delayed dispatch, polling, locks, observers, caches, or new mutable state. It separates output-stream lifetime from t…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS: The authoritative PR diff changes only terminal output-stream and viewport-lease behavior in MobileShellComposite.swift and GhosttySurfaceRepresentable.swift. It adds no NSWindow, `NSPanel…
Cmux Source Artifacts ✅ Passed The PR changes only two tracked Swift source files under Packages/iOS/.... The diff adds and updates hand-written stream and viewport-lease logic. It adds no logs, screenshots, recordings, temporary…
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS. The PR changes only two production Swift files and adds no #if DEBUG test-observation block, debug/test-named member, or wrapper accessor for private state. The new `terminalOutputStream(..., …
Title check ✅ Passed The title clearly and concisely describes the main change: preserving the iPhone viewport lease during output stream churn.
Description check ✅ Passed The description clearly explains the problem, behavior change, ownership rules, and verification results. It omits the template's Demo Video, review trigger, and checklist confirmations, but the core …
Full details: Docstring Coverage

Explanation

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 1 functions across 1 files. (1 skipped: 1 too large.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • 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.

@cursor

cursor Bot commented Sep 22, 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.

Copy link
Copy Markdown
Collaborator Author

@greptileai review

Copy link
Copy Markdown
Collaborator Author

@greptile-apps review

@teamleaderleo
teamleaderleo merged commit 75b7290 into main Sep 22, 2026
59 of 64 checks passed
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