Skip to content

fix(mobile): ask before closing other tabs, sharing desktop's confirm - #33704

Merged
AmoabaKelvin merged 3 commits into
mainfrom
mobile-close-others-confirm
Oct 26, 2026
Merged

AmoabaKelvin merged 3 commits into
mainfrom
mobile-close-others-confirm

Conversation

@AmoabaKelvin

@AmoabaKelvin AmoabaKelvin commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​36 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​35
Prod 9 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​108 $\color{#cf222e}{\Huge{\mathbf{−}}}$​47 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​61

ELI5

On the phone, long-pressing a tab and tapping "Close Other Tabs" closed every other tab right away. One slip of the thumb and they were gone. Now the phone asks "Close other tabs?" first, the same question the desktop app already asks.

What Changed

  • Before: tab long-press sheet → "Close Other Tabs" → every other unpinned tab closes immediately.
  • After: tab long-press sheet → "Close Other Tabs" → the existing mobile confirm sheet asks "Close other tabs?" with "The selected tabs will be closed. Pinned tabs will stay open." Cancel keeps everything. "Close tabs" closes the others. Single-tab "Close" is still immediate.

How:

  • src/shared/bulk-tab-close-confirmation.ts now owns the rule: ask only when at least one tab would close, and after the confirm, close only the tabs that are still in the selection (a remote update can pin, move or close tabs while the dialog is open). The English wording lives here too.
  • Desktop's useBulkTabCloseConfirmation calls that helper and translates the shared English, so its behavior doesn't change. Because the fallbacks are no longer string literals, the six keys move into en-runtime-required.json, the same route background-task-copy takes.
  • Mobile's "Close Other Tabs" action (mobile-bulk-close-sheet-actions.ts) runs through the same helper. It uses closeBeforePress so the confirm sheet opens only after the action sheet is gone (iOS can't present two native modals at once). The confirm state lives in use-mobile-session-bulk-close.ts and renders with the existing ConfirmModal.
  • Every tab type's sheet (terminal, chat, browser, markdown, file) gets this action from that one factory, so all of them ask now.

"Close Tabs to the Right", the other half of the issue, was removed from mobile in #31599, so nothing is left to change there.

Why

Desktop has asked before bulk closes for a while. Mobile skipped the question because its sheet action called the close loop directly. The rule lives in shared code now, so phone and desktop can't drift apart on when to ask or what to say.

Community PR #32473 also adds a confirm. I didn't build on it. It writes its own mobile-only wording and selection rule, which leaves a second copy of desktop's rule, and it rewrites unrelated destructuring across MobileSessionSheets.tsx to stay under the 400-line file limit. This PR shares the rule and adds two lines to that file.

Linked Issue

Fixes #32468

Linear: STA-11868

Visual Proof

iOS simulator (iPhone, iOS 27). This is a small route that renders the real useMobileSessionBulkClose hook, ActionSheetModal and ConfirmModal with four fake terminal tabs. The same route was run on main and on this branch.

BEFORE (main): Close Other Tabs closes the other three tabs at once.

mcoc-before.mp4

AFTER: the confirm sheet appears. Cancel keeps all four tabs. Confirming leaves only "Build".

mcoc-after.mp4

Testing

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

  • New mobile-bulk-close-sheet-actions.test.ts: pressing "Close Other Tabs" closes nothing until the confirm resolves, then closes the other tabs. It fails on main.

  • The existing desktop tests (useTabGroupCloseScopeCommands.test.tsx, terminal-workspace-keydown.test.ts) cover the shared helper's cancel, pinned-while-pending and moved-tab cases. They pass unchanged.

  • verify:localization-runtime-catalog, check:code-quality:changed and check:react-doctor:changed pass.

  • Manually tested on the iOS simulator (videos above). The iOS log showed no "already presenting" warnings when the confirm opened after the sheet.

AI Disclosure

Review

  • Desktop behavior should be unchanged. use-bulk-tab-close-confirmation.ts now hands its old inline flow to the shared helper.
  • The mobile confirm is a promise resolved from ConfirmModal. Backdrop close and Cancel both resolve false.
  • The change is client-only, with no wire or host changes, so phones and desktops on different versions are unaffected.

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance

The phone-side change is the same on iOS, Android and the mobile web page, since they share the session sheets. Nothing here touches SSH, git or folder workspaces.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@AmoabaKelvin
AmoabaKelvin force-pushed the mobile-close-others-confirm branch from ad9765c to ebe787c Compare October 26, 2026 19:40
@AmoabaKelvin
AmoabaKelvin marked this pull request as ready for review October 26, 2026 19:40
@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4fedd5ff-3373-4ec4-a925-2f1a3640c49c
Commits

Reviewing files that changed from the base of the PR and between 2451037 and ebe787c.

Files selected for processing: 11
  • mobile/src/session/MobileBrowserTabActionSheet.tsx
  • mobile/src/session/MobileSessionSheets.tsx
  • mobile/src/session/mobile-bulk-close-sheet-actions.test.ts
  • mobile/src/session/mobile-bulk-close-sheet-actions.ts
  • mobile/src/session/mobile-terminal-action-sheet-actions.ts
  • mobile/src/session/use-mobile-session-bulk-close.ts
  • src/renderer/src/components/tab-group/useTabGroupCloseScopeCommands.test.tsx
  • src/renderer/src/components/tab-group/useTabGroupCloseScopeCommands.ts
  • src/renderer/src/components/use-bulk-tab-close-confirmation.ts
  • src/renderer/src/i18n/en-runtime-required.json
  • src/shared/bulk-tab-close-confirmation.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Walkthrough

The change adds a shared helper for confirming bulk tab closures and reselecting tabs before closing. The renderer confirmation hook now uses this helper. Mobile bulk-close actions now request confirmation, and the session sheets display a confirmation modal. A mobile test checks that the selected tabs close only after confirmation.

Priority: Normal

Severity of issue fixed: Medium

Merge Risk: Minimal · up to ebe78

Mobile "Close Other Tabs" now asks for confirmation before closing, and cancelling leaves tabs open. No concrete merge-blocking issue was found.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 10 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the main change: adding confirmation before mobile users close other tabs and sharing the desktop confirmation behavior.
Description check Passed The description is complete and directly aligned with the template. It explains the user-facing change, implementation, rationale, linked issue, visual proof, testing, review notes, compatibility cons…
Linked Issues check Passed Issue #32468 requires confirmation for the destructive mobile bulk action while keeping single-tab Close immediate. createBulkCloseSheetActions now uses closeTabsAfterConfirmation, sets `closeBefo…
Out of Scope Changes check Passed The shared confirmation helper, desktop integration, runtime localization entries, mobile wiring, and tests directly support issue #32468. The shared scopes preserve existing desktop behavior and prev…
Full details: Description check

Explanation

The description is complete and directly aligned with the template. It explains the user-facing change, implementation, rationale, linked issue, visual proof, testing, review notes, compatibility considerations, and checklist status. The standard lint, typecheck, test, and build commands are not marked as passing, but this is a non-critical omission because other validation and manual testing are documented.

Full details: Linked Issues check

Explanation

Issue #32468 requires confirmation for the destructive mobile bulk action while keeping single-tab Close immediate. createBulkCloseSheetActions now uses closeTabsAfterConfirmation, sets closeBeforePress, and closes only confirmed tab IDs that still exist. useMobileSessionBulkClose renders the existing ConfirmModal model with the required title, message, and confirm label. The shared factory supplies the action to terminal, chat, browser, markdown, and file sheets. The added mobile test verifies that tabs remain open before confirmation and close after confirmation. The PR reports that Close Tabs to the Right is no longer a mobile action, so no unconfirmed right-side action remains.

Full details: Out of Scope Changes check

Explanation

The shared confirmation helper, desktop integration, runtime localization entries, mobile wiring, and tests directly support issue #32468. The shared scopes preserve existing desktop behavior and prevent mobile and desktop confirmation rules from diverging. No unrelated product behavior appears in the reviewed changes.

Full details: Docstring Coverage

Explanation

Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 10 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

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
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@AmoabaKelvin
AmoabaKelvin merged commit d656786 into main Oct 26, 2026
96 of 98 checks passed
@AmoabaKelvin
AmoabaKelvin deleted the mobile-close-others-confirm branch October 26, 2026 20:15
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.

ux(mobile): require confirmation before executing Close Other Tabs in tab long-press action sheet

1 participant