Skip to content

fix(mobile): close terminal session tabs authoritatively - #11240

Merged
brennanb2025 merged 1 commit into
mainfrom
brennanb2025/mobile-6927-close-tabs
Jul 30, 2026
Merged

brennanb2025 merged 1 commit into
mainfrom
brennanb2025/mobile-6927-close-tabs

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Jul 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Route terminal long-press Close through the authoritative session-tab close path when the terminal handle belongs to a session tab.
  • Keep the handle-only close as a fallback for live handles that have no session-tab owner.
  • Cover single terminals, split-leaf ownership, and the fallback path.

Fixes #6927
Fixes #7345

Merge ordering

Depends on #11251 (brennanb2025/mobile-9717-dup-sessions, issue #9717) — land #11251 first.

This PR correctly routes more terminal closes through the authoritative
session.tabs.close path, which prunes sessionTabs and clears
activeSessionTabId. That legitimately reaches the empty-session-tab-list state
far more often, and on main that state can trigger an unsolicited replacement
terminal — the duplicate-session bug in #9717. Landing this PR first therefore
makes #9717 more reachable without introducing it.

#11251 fixes the auto-create side (use-initial-session-terminal-autocreate.ts)
by treating an authoritative empty snapshot as a real empty workspace. The two
branches combine cleanly and do not require an atomic merge, but this PR should
not land before #11251. #11251 records the same ordering constraint.

Root cause

Terminal long-press Close was the only mobile tab close that called handle-only terminal.close. That removed the PTY-facing terminal but did not prune the mobile sessionTabs, clear activeSessionTabId, or record the local close tombstone. The host could therefore republish the same active tab as pending, after which the pending-terminal activation effect materialized a fresh PTY into the same tab id.

#7345 reports this exact terminal resurrection path. #6927 sees it as every tab being unclosable because a headless orca serve session normally publishes terminal tabs; #10486 is separate because its trigger is viewing/activation and does not exercise close.

Reliability contract

  • Invariant (terminal-session.explicit-close-retirement): an explicit mobile terminal-tab close uses the session-tab owner transaction, removes the local mirror, clears active ownership, and records the tombstone; an unmatched live handle retains the existing fallback.
  • Failure source: [Bug]: Mobile cannot close tabs #6927 and [Bug]: Terminal closed from mobile app reappears as a brand-new terminal #7345.
  • Oracle: action-sheet tests assert the matching session tab is closed, the owning split leaf is selected, and unmatched handles use the fallback. Replacing the new routing branch with the old handle-only call makes 2 of 4 focused tests fail.
  • Gate: existing terminal-session.explicit-close-retirement is partial; this PR adds the mobile route-level oracle and records live-device validation as a gap.
  • Provider/platform coverage: the pure mobile routing contract is provider- and path-agnostic, so local, daemon, SSH, WSL, folder workspaces, and git worktrees are unaffected below the existing close RPC. The full suite and static gates ran on macOS; a physical phone/relay journey was not run.
  • Performance budget: one bounded session-tab lookup occurs when the action sheet is built and is reused for bulk-close anchoring. No polling, provider inventory, render loop, listener, subprocess, or startup work is added.
  • Diagnostics: focused assertion names distinguish owner routing, split selection, and fallback behavior.
  • Residual gap: mobile does not interpret RuntimeMobileSessionTabCloseResult.refused, but session.tabs.close hardcodes user intent and cannot currently reach a refusal branch. Handling a future user-close refusal is intentionally out of this P1 scope.

Verification

Against parent d3681f6306:

  • Parent: 351 test files, 2,590 passed, 2 skipped
  • This PR: 351 test files, 2,593 passed, 2 skipped
  • cd mobile && pnpm install using the repository-pinned pnpm 10.24.0
  • Full mobile Vitest suite with --maxWorkers=2
  • Full mobile oxlint
  • Full mobile oxfmt --check .
  • Mobile tsc --noEmit
  • Non-vacuity mutation: old routing produces 2 focused failures

No device validation is claimed.

CI note (pre-existing unrelated flake)

tests 12/16 failed once on this commit. All 233 shard files and 2,247 tests
passed; the job exited 1 solely on Unhandled Rejection: Error: Unable to remove watcher: Invalid argument, thrown from @parcel/watcher's Linux
inotify backend (inotify_rm_watch -> EINVAL).

Not caused by this PR:

  • The root Vitest config's include matches no file under the repo-root
    mobile/ directory, so this mobile-only diff leaves shard 12's file set
    byte-identical to the parent — it cannot shift shard contents.
  • Re-running the same job on the identical tree passed.
  • The blamed file (src/main/plugins/plugin-content-safety.test.ts) contains no
    watcher code; Vitest attributes an async native error to whichever test was
    in flight.

The latent hazard is in src/main/ipc/parcel-watcher-in-process-fallback.ts:
the cancellation path deliberately swallows unsubscribe failures ("so they never
surface as unhandled rejections"), but the normal teardown path awaits
subscription.unsubscribe() unguarded. Desktop main-process code, unrelated to
this mobile fix, and intentionally left out of scope here.

@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes terminal "tab resurrection" bugs (#6927, #7345) on mobile by routing the long-press Close action through the authoritative session.tabs.close path instead of the handle-only terminal.close path when the pressed terminal belongs to a session tab.

  • mobile-terminal-action-sheet-actions.ts: Adds onCloseSessionTab callback; computes sessionTab once at action-build time and reuses it for both the Close routing and bulkCloseActions anchor; uses a generic Tab extends TerminalTab to preserve the concrete type through the callback.
  • [worktreeId].tsx: Wires onCloseSessionTab to handleCloseSessionTab, which runs the full teardown — session.tabs.close RPC, tombstone recording, sessionTabs pruning, and activeSessionTabId clearance.
  • mobile-terminal-action-sheet-actions.test.ts: Adds a buildActions helper and three focused tests covering the authoritative path, split-leaf targeting, and the handle-only fallback.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to the terminal long-press Close path and is backed by targeted tests that include a non-vacuity mutation check.

The fix is a small, focused routing change: one new callback wired from the session route, one lookup moved earlier, and a two-branch conditional in the Close handler. All three branches (authoritative owner, split-leaf owner, and unmatched fallback) are covered by the new tests, and the PR description confirms that reverting the routing branch breaks two of the four focused tests. The handleCloseSessionTab function itself was already in production use for other tab types, so no new teardown logic is introduced.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename Overview
mobile/src/session/mobile-terminal-action-sheet-actions.ts Adds onCloseSessionTab callback, moves sessionTab lookup before the action list, and routes Close through the session-tab owner when found; clean and correct.
mobile/app/h/[hostId]/session/[worktreeId].tsx Single-line wiring of onCloseSessionTab to the existing handleCloseSessionTab function; minimal, correct change.
mobile/src/session/mobile-terminal-action-sheet-actions.test.ts Introduces buildActions helper and three new focused tests; covers authoritative path, split-leaf ownership, and fallback; no issues.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User long-presses terminal\nand taps Close] --> B[getMobileTerminalActionSheetActions\nonPress handler]
    B --> C[onDismiss: close action sheet]
    C --> D{sessionTab found?\ntabs.find tab.terminal === handle}
    D -- Yes --> E[onCloseSessionTab tab\nsession.tabs.close RPC]
    E --> F[Prune sessionTabs\nRecord tombstone\nClear activeSessionTabId\nUnsubscribe terminal handle]
    D -- No --> G[onClose target\nterminal.close RPC]
    G --> H[Remove from local terminals list\nClear activeHandle]
Loading

Reviews (1): Last reviewed commit: "fix(mobile): close terminal session tabs..." | Re-trigger Greptile

@coderabbitai

coderabbitai Bot commented Jul 28, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The terminal action sheet now accepts typed session tabs and an onCloseSessionTab callback. Its Close action finds the tab owning the target handle, closes that tab when found, and otherwise uses the existing handle-only callback. The session screen passes handleCloseSessionTab into the action sheet. Tests add shared action construction and cover owned-tab closure, split-leaf selection, and fallback behavior.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers summary and verification, but it omits required template sections like Screenshots, AI Review Report, Security Audit, and Notes. Add the missing template sections, including screenshots or 'No visual change', a testing checklist, AI Review Report with macOS/Linux/Windows coverage, Security Audit, and Notes.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The code routes mobile terminal close through the authoritative session-tab path and preserves the handle-only fallback, matching #6927 and #7345.
Out of Scope Changes check ✅ Passed The diff stays focused on the mobile close-path fix and its tests, with no obvious unrelated code changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title is concise, specific, and matches the main change: routing terminal session-tab closes through the authoritative path.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Root cause: terminal long-press Close alone used handle-only terminal.close, leaving the session-tab mirror, active id, and tombstone state behind so pending activation could materialize a fresh PTY. This PR resolves the handle owner once and routes matching terminals—including split leaves—through authoritative session.tabs.close, preserving handle-only fallback for genuinely unmatched handles. Verified on final SHA: parent 351 files / 2,590 passed / 2 skipped → PR 351 / 2,593 / 2; full mobile suite (--maxWorkers=2), oxlint, oxfmt --check, and tsc --noEmit pass, old routing makes 2/4 focused tests fail, and the same-model nested review returned CLEAN with targeted host split/SSH tests green. No physical phone/simulator journey was run; the current unrelated CI failure is the plugin-content-safety watcher teardown shard (its tests passed before teardown).

@brennanb2025
brennanb2025 merged commit 1cc60be into main Jul 30, 2026
49 of 51 checks passed
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
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.

[Bug]: Terminal closed from mobile app reappears as a brand-new terminal [Bug]: Mobile cannot close tabs

1 participant