Repository navigation
refactor(runtime): name every mobile-session close outcome so the tombstone decision is explicit (STA-4718) - #15346
Conversation
…bstone decision is explicit (STA-4718)
📝 WalkthroughWalkthroughThe change adds branded mobile session tab-close outcomes with constructors for committed, delegated, and refused closures. Runtime close paths now use these constructors and preserve refusal reasons and snapshot-republication state. Committed closures tombstone selected tabs, while delegated closures leave selection state unchanged. Tests cover delegated terminal closure, renderer notification, PTY retention, and session-tab retention. Merge Risk: ⚪ Minimal · up to This PR makes mobile-session close outcomes explicit without changing wire behavior or tombstone decisions. The delegated and refused paths remain protected from incorrect tombstones, and no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/runtime/mobile-session-tab-close-outcome.ts (1)
4-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the rationale comments.
These comments explain a valid non-obvious rule, but the blocks are not concise. Keep one brief comment for each tombstone rule. Move the full rationale to design documentation.
As per coding guidelines, comments must be concise, non-obvious, and brief; prefer one line and do not explain obvious behavior or walk through code.
Proposed comment-only cleanup
-// Why: a selection tombstone ... -// ...full rationale... +// Only host-committed closes tombstone client selection. -// Why: the required brand ... -// ...full rationale... +// The private brand requires named outcome constructors. -// Why: the host tore the tab down ... -// ...full rationale... +// Host-committed closes may tombstone client selection. -// Why: the close was handed to the renderer ... -// ...full rationale... +// Delegated closes do not tombstone because the outcome is unknown. -// Why: `snapshotRepublished` tells the client ... +// Include this flag only when the refusal republishes a snapshot.Also applies to: 12-17, 26-28, 38-41, 46-47
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c726e68-2918-4cc5-a83f-03c2bedd771c
📒 Files selected for processing (3)
src/main/runtime/mobile-session-tab-close-outcome.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
Cross-platform verificationRun on real hosts via Linux — Ubuntu 24.04 (kernel 7.0.0), node v26.1.0, pnpm 10.24.0, over SSH. Scope:
On Windows this suite does not collect at all ( Linux gives full coverage of the change: all 1183 tests pass, including the four |
…bstone decision is explicit (STA-4718) (stablyai#15346)
…bstone decision is explicit (STA-4718) (stablyai#15346)
…bstone decision is explicit (STA-4718) (stablyai#15346)
ELI5
When you close a terminal tab from your phone or from the web, the desktop app has three possible answers:
Answer 1 had a proper name in the code. Answers 2 and 3 were anonymous — just bare little objects scattered across about eight places. Nothing said which of the three any given spot meant.
That matters because answer 1 does an extra bit of bookkeeping: it writes down "this tab is gone, stop showing it to that phone." Answers 2 and 3 deliberately don't. But since they had no name, nobody could tell whether that missing bookkeeping was a careful decision or a slip — and someone filed a bug report saying it was a slip.
It wasn't. Doing it there would have caused a real bug: the desktop is allowed to say "no" to a close (say, you cancelled the "this tab is still running something, really close it?" box). If we'd already written down "that tab is gone," your still-running terminal would disappear from your phone permanently, with no way to bring it back.
So this PR doesn't make the change the bug report asked for. It gives all three answers names, so nobody has to guess again.
What you might notice: nothing at all. Every one of those eight places produces exactly the same result as before, down to the order of the fields.
Relates to STA-4718. Follow-up from #13387.
The ticket as filed is invalid — please read before reviewing
STA-4718 asks that two renderer-mediated close branches in
closeMobileSessionTab(orca-runtime.ts, the tworeturn { closed: true }sites) be routed throughfinishCommittedClose()so they record selection tombstones. Doing that would introduce a bug. This PR deliberately does not do it.The omission is load-bearing. Both branches call
this.notifier.closeTerminal(...), which is declaredvoid— fire-and-forget — and lands in the renderer ascloseTerminalTab(tabId, { skipRunningProcessConfirm: true }). That renderer call can legitimately decline to close:terminal-tab-actions.ts:85-88), orconfirmClosePinnedTabon, which opens a modal the user can cancel (terminal-tab-actions.ts:90-112).The host never learns the outcome. And a tombstone is not a passive "don't re-select" hint —
projectWithoutClosedClientSessionTabsfilters the tab out of every snapshot published to that client, and the only thing that clears one isactivate()on that tab id, which a client cannot do for a tab it can no longer see. So tombstoning an uncommitted close would permanently hide a live terminal from a paired client, with no recovery path.This is confirmed by history:
0e96b82e449introducedfinishCommittedClose(), converted the other branches to it, and deliberately left these two alone in the same diff — addingexpect(forgetTabs).not.toHaveBeenCalled()to pin the behaviour. One of its sub-commits is literally "fix(mobile): avoid tombstones for uncommitted closes".The residual symptom the ticket describes — a stale snapshot briefly re-selecting the tab — is real but self-healing, and on the delegated path it is correct: if the renderer refused, the tab really is still there.
What this PR fixes instead
The real defect is narrower and is why the ticket got filed against correct code:
closeMobileSessionTabhas three distinct outcome classes — host-committed, delegated-to-renderer (outcome unknown), and refused — but only the committed one had a named constructor. The other two were bare object literals across ~8 return sites. Nothing at the type level or in review distinguished "I chose not to tombstone" from "I forgot to".This adds named constructors for all three, so every return site declares its class and the deliberate no-tombstone decision is self-documenting.
Behaviour preservation
Strictly behaviour-preserving; the existing tests are the guard and all pass unmodified.
snapshotRepublishedconditionalities (missing-intent=true,unknown-liveness=Boolean(snapshot),stale-publication=Boolean(snapshot),stale-terminal=true,live-host-pty=!addressedDeadLeaf,retirement-owner=key absent).refusedMobileSessionTabClosespreads the key only when truthy, so nosnapshotRepublished: falseever reaches the wire.RuntimeMobileSessionTabCloseResultis serialized to mixed-version clients over RPC, so the brand is type-only (declare const … unique symbol, never assigned) —src/shared/runtime-types.tshas zero diff.expect(forgetTabs).not.toHaveBeenCalled()assertions intact and unweakened.Tests
1183 pass in
orca-runtime.test.ts, plus a new test asserting the delegated path still records no tombstone, phrased around the outcome class rather than the line number.