Repository navigation
fix(mobile): after closing a tab, return to the previous one - #15220
apple-ouyang wants to merge 6 commits into
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdded shared MRU tracking and successor-selection utilities for session tabs. Updated mobile session state, startup, snapshot application, switching, and close actions to track recent tabs and select MRU successors. Updated runtime close and active-surface fallback paths to use recent group history. Added tests for utility behavior and active versus background mobile tab closes. Priority: ➖ Normal Severity of issue fixed: Medium
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 9.52% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 20 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check | ✅ Passed | Issue [#15219] requires previous-tab restoration, most-recently-added fallback, and no selection change for background closes. The PR records phone-side visits, including caller-only activations. It p… |
| Out of Scope Changes check | ✅ Passed | The changes remain within issue [#15219]. Shared successor utilities, runtime history propagation, stale-history pruning, close-path updates, and regression tests directly support the requested mobile… |
| Title check | ✅ Passed | The title clearly and concisely describes the main user-facing change: returning to the previous mobile tab after closing the current tab. |
| Description check | ✅ Passed | The description is mostly complete. It explains the user impact, implementation, motivation, linked issue, visual-proof status, and testing results. The template sections for Review, Notes, and Checkl… |
✨ Finishing Touches 💡 1
⚔️ Resolve merge conflicts 💡
- Resolve merge conflict in branch
fix/mobile-close-tab-successor
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mobile/src/session/mobile-session-last-tab-close.test.ts (1)
23-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse behavior assertions for this regression.
These assertions only check that identifier strings exist in the route source. They can pass when the handler uses incorrect arguments or never reaches the branch. Add a behavior-level test that closes the active and background tabs and asserts active-state clearing and successor activation. If mounting the full screen is difficult, extract and test the pure close-selection decision.
Source basis: the PR objective requires regression coverage for previous-tab and fallback selection.
Also applies to: 28-35
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 55fb6c7f-f40f-41ec-8a59-48dc0a882757
📥 Commits
Reviewing files that changed from the base of the PR and between 13b10e0 and 810fa1e21255c7ad011e1bbe8bf338831784064f.
📒 Files selected for processing (6)
mobile/app/h/[hostId]/session/[worktreeId].tsxmobile/src/session/mobile-session-last-tab-close.test.tssrc/main/runtime/mobile-session-terminal-retirement.tssrc/main/runtime/orca-runtime.tssrc/shared/session-tab-close-successor.test.tssrc/shared/session-tab-close-successor.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review.
810fa1e to
5052aee
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 9be40f7d-9cc8-4315-9228-726acbce5cd6
📥 Commits
Reviewing files that changed from the base of the PR and between f952f1a and 5052aee50bfbf3b5f3431c4bdc2d3cd0e96cf23b.
📒 Files selected for processing (11)
mobile/src/session/active-session-tab.tsmobile/src/session/mobile-session-last-tab-close.test.tsmobile/src/session/use-mobile-session-close-actions.tsmobile/src/session/use-mobile-session-screen-state.tsmobile/src/session/use-mobile-session-startup.tsmobile/src/session/use-mobile-session-tab-application.tsmobile/src/session/use-mobile-session-tab-switching.tssrc/main/runtime/mobile-session-terminal-retirement.tssrc/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.tssrc/shared/session-tab-close-successor.test.tssrc/shared/session-tab-close-successor.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/shared/session-tab-close-successor.test.ts
- src/main/runtime/mobile-session-terminal-retirement.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 1f5596c1-0531-4e70-9caf-4f777fa7a46f
📥 Commits
Reviewing files that changed from the base of the PR and between 5052aee50bfbf3b5f3431c4bdc2d3cd0e96cf23b and 1da2dc99957132b9953db096acc5e1d6cc736f01.
📒 Files selected for processing (15)
mobile/src/session/active-session-tab.test.tsmobile/src/session/active-session-tab.tsmobile/src/session/mobile-session-route-parity.test.tsmobile/src/session/mobile-session-route-types.tsmobile/src/session/use-mobile-session-tab-application.tssrc/main/runtime/client-session-tab-closed-selection.tssrc/main/runtime/mobile-session-terminal-retirement.test.tssrc/main/runtime/mobile-session-terminal-retirement.tssrc/main/runtime/orca-runtime-apply-mobile-session-tab-navigation.tssrc/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.test.tssrc/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.tssrc/main/runtime/runtime-mobile-session-result-finalization.tssrc/shared/runtime-session-contracts.tssrc/shared/session-tab-close-successor.test.tssrc/shared/session-tab-close-successor.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- mobile/src/session/use-mobile-session-tab-application.ts
- mobile/src/session/active-session-tab.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Preserve recentTabIds during hydration… · orca-runtime-apply-mobile-session-tab-navigation.ts:48-55
src/main/runtime/orca-runtime-apply-mobile-session-tab-navigation.ts:48-55
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve
recentTabIdsduring hydration rebuilds.When navigation stores global history, a later graph sync can call
hydrateHeadlessMobileSessionTabsFromWorkspaceSession. The rebuiltnextSnapshotomitsrecentTabIds, then replaces the existing snapshot. A later cross-group close therefore loses global MRU ordering and can selectb-oldfrom group history instead ofa-new. Carryexisting.recentTabIdsintonextSnapshot.Suggested fix
: {}), - tabs: mergedTabs + tabs: mergedTabs, + ...(existing?.recentTabIds + ? { recentTabIds: [...existing.recentTabIds] } + : {}) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: a15e83ef-faeb-4e1f-b3e3-58f7c14a5f86
📥 Commits
Reviewing files that changed from the base of the PR and between 1da2dc99957132b9953db096acc5e1d6cc736f01 and f0a1bedd6fb1658b7766e743bf0658a9d8a2664d.
📒 Files selected for processing (5)
src/main/runtime/mobile-session-terminal-retirement.test.tssrc/main/runtime/mobile-session-terminal-retirement.tssrc/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.test.tssrc/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.tssrc/shared/session-tab-close-successor.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.ts
- src/main/runtime/mobile-session-terminal-retirement.test.ts
- src/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.test.ts
- src/main/runtime/mobile-session-terminal-retirement.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Closing a document on the phone cleared the active tab and the next snapshot fell back to the leftmost remaining tab. Desktop already prefers the previously viewed tab. Phone switches are caller-only, so that visit history never reached the host. Remember viewed tabs on the phone. On close, activate the previous visit if it is still open; otherwise activate the most recently added remaining tab. Example: - Input: viewed Terminal, then Doc A, then Doc B; close Doc B - Before: jumps to the leftmost tab - After: returns to Doc A
Add an authoritative cross-group MRU (`recentTabIds`) to RuntimeMobileSessionTabsSnapshot and its public result: recorded on activation (including caller-only activations that never focus the host window), pruned against surviving top-level tabs on close and projection, and consulted before the per-group MRU merge in both successor paths (closeHeadlessMobileTerminalTab and retireTerminalSurfacesFromSnapshot). The per-group merge stays as the fallback for snapshots that predate the field. Also drop the leftmost-tab fallback in resolveActiveSessionTab (a snapshot with no active tab now picks the most recently added remaining tab) and prune mobile recent history only after orphaned dirty drafts are appended, so closing another tab can still return to a draft. Bump the mobile route parity snapshots for the hook extraction changes.
Regression tests for the review round: the cross-group close and retirement paths must follow the global visit history (a-new wins over the older b-old / first-group pick), the close path prunes recentTabIds, resolveActiveSessionTab falls back to the most recently added tab when the snapshot has no active tab, and pruneRecentTabIds drops stale entries. Each was red against the pre-fix tree.
Closing the active tab must not resurrect an older group sibling over a newer cross-group visit: chooseActiveSurface now keeps a surviving active surface first, then consults the global visit history (per-group merge for older snapshots), before the repaired group fallback. Both snapshot filters now use pruneRecentTabIds and assign the result explicitly, so an emptied history falls back to the group history instead of silently selecting the last-added tab, and stale ids no longer survive through conditional spreads.
…tory fallback Regression coverage for the two late review findings: retiring the active tab keeps the newer cross-group visit over the repaired group sibling, and a history that filters to empty falls back to the group history (and drops its stale ids) instead of the last-added tab.
f660daf to
89261f4
Compare
ELI5
If you close a document on your phone, Orca used to jump to the leftmost tab. It now goes back to the tab you were looking at before, and only if you never visited another tab does it land on the newest remaining one.
What Changed
tabs[0]as the snapshot / headless-close fallback.Why
Phone switches do not update host
recentTabIds. Close also cleared the local active tab, so the next snapshot picked the leftmost remaining tab.Linked Issue
Fixes #15219
Visual Proof
N/A — mobile session tab selection after close. Covered by unit tests for previous-visit vs newest-remaining successor selection.
Testing
vitest run --config config/vitest.config.ts src/shared/session-tab-close-successor.test.ts src/main/runtime/mobile-session-terminal-retirement.test.ts src/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.test.ts— 18 passed.mobile/session suite (vitest run src/session/) — 191 files / 1828 tests, all passing after theorigin/mainmerge (route-parity fingerprint pins re-recorded against the merged tree).pnpm run typecheck— clean;oxlint+oxfmtclean on the touched files.