Repository navigation
fix(mobile): closing the tab a phone shows returns it to the tab it viewed before - #31365
Conversation
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
Issue #15219 requires the most recently added remaining tab when no previously viewed tab remains. projectClientSessionTabSelection delegates this case to pickNextActiveTabAfterClose, which select… |
Change the no-history fallback to select the last remaining tab in strip order, or otherwise match issue #15219. Add a test where the right-hand neighbour and the last remaining tab differ. |
| Docstring Coverage | |
Docstring coverage is 12.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 18 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary behavior change: closing the tab shown on a phone returns to the previously viewed tab. Description check The description is complete and well aligned with the template. It explains the user impact, implementation, rationale, linked issue, visual proof, testing, compatibility considerations, and checklist… Out of Scope Changes check The host selection, persistence, migration, projection, and test changes support issue #15219. The refactor and layout-oracle updates support the shared selection behavior. No unrelated change is demo…Full details: Description check
-
Explanation
The description is complete and well aligned with the template. It explains the user impact, implementation, rationale, linked issue, visual proof, testing, compatibility considerations, and checklist status.
Full details: Out of Scope Changes check
-
Explanation
The host selection, persistence, migration, projection, and test changes support issue
#15219. The refactor and layout-oracle updates support the shared selection behavior. No unrelated change is demonstrated.
-
Full details: Linked Issues check
-
Explanation
Issue
#15219requires the most recently added remaining tab when no previously viewed tab remains.projectClientSessionTabSelectiondelegates this case topickNextActiveTabAfterClose, which selects the closed tab's right-hand neighbour before the last remaining tab. The current fallback test uses['a', 'b', 'c', 'd']and closesc, so both results ared. The implementation and tests do not satisfy the required fallback for cases where the neighbour differs from the last tab.Resolution
Change the no-history fallback to select the last remaining tab in strip order, or otherwise match issue
#15219. Add a test where the right-hand neighbour and the last remaining tab differ.
✨ 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.
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
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
a577d8f7-b552-42aa-850a-53308cad9616
📒 Files selected for processing: 6
mobile/src/session/mobile-markdown-document-lifetime.test.tsmobile/src/session/use-mobile-session-close-actions.test.tsmobile/src/session/use-mobile-session-close-actions.tsmobile/src/session/use-mobile-session-screen-state.tsmobile/src/test-support/rpc-recording/adapters/session-screen-tab-mount-adapters.tsmobile/src/test-support/rpc-recording/adapters/session-terminal-create-mount-adapters.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.
| @@ -0,0 +1,151 @@ | |||
| import { describe, expect, it, vi } from 'vitest' | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
set -eu
printf '%s\n' '--- revisions ---'
git rev-parse --show-toplevel
git cat-file -e 411b5c024abef11dcc269779377bc7d3aefa897c^{commit}
git cat-file -e 264f8c273fd1f3e1d947e58cd73bb83c87f52a77^{commit}
printf '%s\n' '--- changed file diff ---'
git diff --no-ext-diff --unified=80 411b5c024abef11dcc269779377bc7d3aefa897c 264f8c273fd1f3e1d947e58cd73bb83c87f52a77 -- mobile/src/session/use-mobile-session-close-actions.test.ts
printf '%s\n' '--- current file ---'
nl -ba mobile/src/session/use-mobile-session-close-actions.test.ts
printf '%s\n' '--- directly bound scripts/config references ---'
rg -n -F --glob 'package.json' -e '"typecheck:tests"' -e '"typecheck"' mobile package.json || test "$?" -eq 1
rg -n -F --glob '*tsconfig*.json' -e 'use-mobile-session-close-actions.test.ts' -e 'src/session' mobile || test "$?" -eq 1
find mobile -maxdepth 3 -type f \( -name 'tsconfig*.json' -o -name 'package.json' \) -printRepository: stablyai/orca
Length of output: 13962
🏁 Script executed:
set -eu
printf '%s\n' '--- mobile test tsconfig ---'
nl -ba mobile/tsconfig.test.json
printf '%s\n' '--- fixture and hook-mount definitions ---'
rg -n -F --glob '*.ts' -e 'function mountFixture' -e 'const mountFixture' -e 'export.*mountFixture' -e 'function hookMount' -e 'export.*hookMount' -e 'function performHookAction' mobile/src mobile/test-support || test "$?" -eq 1
sed -n '1,240p' mobile/src/test-support/rpc-recording/recorder-fixture-shape.ts
sed -n '1,240p' mobile/src/test-support/rpc-recording/hook-mount.ts
printf '%s\n' '--- hook declarations ---'
rg -n -F --glob '*.ts' -e 'export function useMobileSessionCloseActions' -e 'export function useMobileSessionTabApplication' -e 'export function useMobileSessionTabSwitching' -e 'type SessionScope' -e 'interface SessionScope' mobile/src/session
printf '%s\n' '--- relevant hook source headers and declarations ---'
sed -n '1,220p' mobile/src/session/use-mobile-session-close-actions.ts
sed -n '1,180p' mobile/src/session/use-mobile-session-tab-application.ts
sed -n '1,180p' mobile/src/session/use-mobile-session-tab-switching.ts
printf '%s\n' '--- RPC response and client declarations ---'
rg -n -F --glob '*.ts' -e 'export type RpcResponse' -e 'export interface RpcResponse' -e 'sendRequest' mobile/src/transport mobile/src/session | head -80Repository: stablyai/orca
Length of output: 1807
🏁 Script executed:
set -eu
printf '%s\n' '--- fixture and hook-mount definitions ---'
nl -ba mobile/src/test-support/rpc-recording/recorder-fixture-shape.ts
nl -ba mobile/src/test-support/rpc-recording/hook-mount.ts
printf '%s\n' '--- hook declarations and relevant source ---'
rg -n -F --glob '*.ts' -e 'export function useMobileSessionCloseActions' -e 'export function useMobileSessionTabApplication' -e 'export function useMobileSessionTabSwitching' -e 'type SessionScope' -e 'interface SessionScope' mobile/src/session || test "$?" -eq 1
nl -ba mobile/src/session/use-mobile-session-close-actions.ts | sed -n '1,240p'
nl -ba mobile/src/session/use-mobile-session-tab-application.ts | sed -n '1,220p'
nl -ba mobile/src/session/use-mobile-session-tab-switching.ts | sed -n '1,220p'
printf '%s\n' '--- RPC response and client declarations ---'
rg -n -F --glob '*.ts' -e 'export type RpcResponse' -e 'export interface RpcResponse' -e 'sendRequest' mobile/src/transport mobile/src/session | head -120Repository: stablyai/orca
Length of output: 42394
🏁 Script executed:
set -eu
printf '%s\n' '--- model declarations ---'
nl -ba mobile/src/session/use-mobile-session-content-create-actions.ts | sed -n '1,260p'
nl -ba mobile/src/session/use-mobile-session-terminal-list.ts | sed -n '1,280p'
nl -ba mobile/src/session/use-mobile-session-keyboard-state.ts | sed -n '1,240p'
printf '%s\n' '--- RPC and operation declarations ---'
nl -ba mobile/src/transport/unvalidated-rpc-request-port.ts | sed -n '1,100p'
nl -ba mobile/src/transport/types.ts | sed -n '1,100p'
nl -ba mobile/src/transport/rpc-operation.ts | sed -n '1,140p'
printf '%s\n' '--- related fixture patterns ---'
rg -n -F --glob '*.test.ts' -e 'Parameters<typeof useMobileSession' -e 'mountFixture<.*Scope' -e 'sendRequest: vi.fn(' mobile/src/session mobile/src/test-support | head -160Repository: stablyai/orca
Length of output: 41522
🏁 Script executed:
set -eu
printf '%s\n' '--- lifecycle declarations for client and state setters ---'
rg -n -F --glob '*.ts' -e 'client:' -e 'setActiveSessionTabId' -e 'activeSessionTabIdRef' mobile/src/session/use-mobile-session-lifecycle.ts mobile/src/session mobile/src | head -180
nl -ba mobile/src/session/use-mobile-session-lifecycle.ts | sed -n '1,260p'
printf '%s\n' '--- exact client type declarations ---'
rg -n -F --glob '*.ts' -e 'export type RpcClient' -e 'export interface RpcClient' -e 'UnvalidatedRpcRequestPort | null' -e 'client: UnvalidatedRpcRequestPort' mobile/src
nl -ba mobile/src/transport/rpc-client.ts | sed -n '1,100p'
printf '%s\n' '--- nearby fixture implementations ---'
nl -ba mobile/src/session/use-mobile-session-document-readers.test.ts | sed -n '1,175p'
nl -ba mobile/src/session/use-mobile-session-tab-switching.test.ts | sed -n '1,100p'Repository: stablyai/orca
Length of output: 40326
🏁 Script executed:
set -eu
printf '%s\n' '--- model chain declarations ---'
for f in mobile/src/session/use-mobile-session-tab-reconciliation.ts mobile/src/session/use-mobile-session-tabs-reconciliation.ts mobile/src/session/use-mobile-session-terminal-create-actions.ts mobile/src/session/use-mobile-session-terminal-stream-display.ts mobile/src/session/use-mobile-session-content-create-actions.ts; do
if test -f "$f"; then
echo "### $f"
rg -n -C 4 -F -- 'setActiveSessionTabId' "$f" || true
rg -n -C 3 -F -- 'client:' "$f" || true
rg -n -F -- 'export type ' "$f" || true
fi
done
printf '%s\n' '--- all direct declarations of the setter ---'
rg -n -C 6 -F -- 'setActiveSessionTabId:' mobile/src/session mobile/src | head -160
printf '%s\n' '--- explicit fixture member types in the new test ---'
nl -ba mobile/src/session/use-mobile-session-close-actions.test.ts | sed -n '30,95p'Repository: stablyai/orca
Length of output: 23238
🏁 Script executed:
set -eu
printf '%s\n' '--- active tab state declarations ---'
rg -n -C 8 --glob '*.ts' -e 'activeSessionTabId.*useState' -e 'useState.*activeSessionTabId' -e 'setActiveSessionTabId' mobile/src/session mobile/src | head -260
printf '%s\n' '--- model files in the inheritance chain ---'
rg -n -F --glob 'use-mobile-session-*.ts' -e 'export type MobileSession' -e 'activeSessionTabId:' -e 'setActiveSessionTabId' mobile/src/session | head -260Repository: stablyai/orca
Length of output: 35852
🏁 Script executed:
set -eu
printf '%s\n' '--- mobile compiler options ---'
nl -ba mobile/tsconfig.json | sed -n '1,180p'
printf '%s\n' '--- exact decisive declarations ---'
nl -ba mobile/src/session/use-mobile-session-screen-state.ts | sed -n '54,76p'
nl -ba mobile/src/transport/rpc-client.ts | sed -n '19,30p'
nl -ba mobile/src/transport/unvalidated-rpc-request-port.ts | sed -n '27,35p'
nl -ba mobile/src/test-support/rpc-recording/recorder-fixture-shape.ts | sed -n '21,40p'Repository: stablyai/orca
Length of output: 4207
Use the raw RPC parameter type in the client fixture.
RpcClient.sendRequest accepts params?: unknown, but the fixture narrows it to { tabId?: string }. Under strict TypeScript, this callback is not assignable. Accept unknown and narrow it before reading tabId. Keep the test in the type-check project.
🐛 Suggested fix
- async (method: string, params?: { tabId?: string }): Promise<RpcResponse> => {
+ async (method: string, params?: unknown): Promise<RpcResponse> => {
if (method === 'session.tabs.activate') {
- activated.push(params?.tabId ?? '')
+ const tabId =
+ typeof params === 'object' &&
+ params !== null &&
+ 'tabId' in params &&
+ typeof params.tabId === 'string'
+ ? params.tabId
+ : ''
+ activated.push(tabId)🧰 Tools
🪛 GitHub Actions: Mobile Checks / Mobile verify
[error] 1-1: Test file no longer typechecks. The test type-check ratchet failed; run 'pnpm --filter orca-mobile typecheck:tests' to see the errors and fix the test rather than adding it to the baseline.
Source: Pipeline failures
| const successorId = pickNextActiveTabAfterClose( | ||
| { tabOrder, recentTabIds: recentSessionTabIdsRef.current }, | ||
| new Set([tab.id]), | ||
| remainingIds, | ||
| new Set(remainingIds) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the requested fallback when visit history has no remaining tab.
If leftmost is active and the tabs are ordered leftmost, doc-a, doc-b, this helper selects doc-a. The linked issue requests the most recently added remaining tab, which is doc-b in this case. Keep the recent-visit preference, but use creation recency for the fallback rather than the right-hand-neighbor rule. The PR objectives explicitly state the “most recently added remaining tab” requirement.
264f8c2 to
e8663bf
Compare
…ewed before The host kept only each paired device's current tab, so when that tab closed (from the phone, the desktop, the CLI or a process exit) the device fell back to its group's first tab. The per-device selection now keeps the tabs that device showed and moves a closed pick with the desktop's close rule, pickNextActiveTabAfterClose.
… desktop's close too When a device's shown group empties, it moves to the group's split sibling, as the desktop's collapseGroupLayout does. A selection saved before devices kept their own recency uses the host group's. The orcad migration keeps a device's recency for the tabs that move.
e8663bf to
e759ed9
Compare
AmoabaKelvin
changed the title
…top's move rule A paired client's group now moves on by that client's recency when its shown tab leaves, as the desktop's tab move does, instead of falling back to the group's first tab.
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 · Recover the active group from the client’s… · client-session-tab-selection-projection.ts:94-110
src/main/runtime/client-session-tab-selection-projection.ts:94-110
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecover the active group from the client’s recency history.
The current projection selects
group.tabOrder[0]when a saved selection has no group entry.forgetTabsclears the closed global selection and preserves the missing group entry, so closing the shown tab can select the wrong remaining tab.Using
selection.recentTabIds?.at(-1)is safe here. The existing helper filters that value through the affected group’s remaining IDs. It therefore handles a closed last-history entry and history that spans groups.Suggested fix
- const selected = selection.activeTabIdByGroupId[group.id] + const selected = + selection.activeTabIdByGroupId[group.id] ?? + (selection.activeGroupId === group.id ? selection.recentTabIds?.at(-1) : undefined)
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
0f6883f3-2242-4136-bd64-ced6a5b04075
📒 Files selected for processing: 2
src/main/runtime/client-session-tab-selection-projection.tstests/e2e/workspace-layout-oracle-headless.spec.ts
🚧 Files skipped from review as they are similar to previous changes: 1
- src/main/runtime/client-session-tab-selection-projection.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
ELI5
When you close the tab your phone is showing, the phone should go back to the tab you looked at before it, the same way the desktop does. It used to jump to the leftmost tab.
What Changed
Before: on the phone, view terminal → Doc A → Doc B, then close Doc B. The phone lands on the terminal (the leftmost tab). The same happens when the desktop, the CLI or a process exit closes the tab the phone is showing.
After: the phone lands on Doc A. If it viewed no other tab that is still open, it lands on the closed tab's right-hand neighbour, else the last tab, which is the desktop's rule.
The fix is on the host. Each paired device's tab selection (
ClientSessionTabSelectionStore) used to keep only the device's current tab. When that tab closed, the projection fell back to the group's first tab. Now:recentTabIds, the top-level tabs that device showed, most recent last. Every tab activation from the device adds to it. Until the device picks a tab, it starts from the host group'srecentTabIds. It is saved with the rest of the selection, so it survives a host restart.pickNextActiveTabAfterClose, fed with that device's recency. There is no new rule.findSiblingGroupId), else the first group, as the desktop'scollapseGroupLayoutdoes.recentTabIdsfor the tabs that move, validates it, and registers it for path rewrites.deriveClientSessionTabSelection,activateClientSessionTabSelectionandprojectClientSessionTabSelectionmoved toclient-session-tab-selection-projection.tsunchanged apart from this fix; the store file had reached the 300-line limit.Why
The phone's tab switches only change that device's selection on the host, so the host never learned which tabs the phone had visited. The earlier version of this PR kept that history on the phone and switched before sending the close. That only covered closes the phone started, and it duplicated the desktop's rule in a second place. Keeping the history in the host's per-device selection fixes every close route and reuses the desktop's rule as it is.
The phone needs no change. After a close it already clears its own pick and shows the tab the host marks active (
mobile/src/session/active-session-tab.ts).Linked Issue
Fixes #15219
Visual Proof
Android emulator, phone on main's page code (this PR changes no phone code), against a hidden dev desktop built from main (BEFORE) and from this branch (AFTER). Both start with a terminal, doc-a.md and doc-b.md open.
Closing the shown tab from the phone: view terminal → doc-a.md → doc-b.md, long-press doc-b.md → Close.
before-phone-close.mp4
after-phone-close.mp4
The desktop closes doc-b.md while the phone shows it (same visits on the phone first):
before-desktop-close.mp4
after-desktop-close.mp4
Testing
client-session-tab-selection.test.ts: the device returns to its previous tab when the desktop closes its shown tab and when it closes the tab itself; the right-neighbour fallback; recency across a host restart; a saved selection from before this change uses the host's recency; an emptied group hands over to its split sibling. Each fails on main or on the first commit.orcad-source-client-state.test.ts: recency moves with the tabs that move and a malformed list is refused.workspace-layout-oracle-headless.spec.ts(orcad and Electron serve): a group whose shown tab moves to another group now moves on by the client's recency, as the desktop's tab move does (tabs-move-actions.ts), so the oracle expects that instead of the group's first tab. The move scenarios pass locally on both hosts.pnpm lint,pnpm tc,oxfmt --check .,check:code-quality:changed,check:react-doctor:changedandcheck:zustand-selector-fanoutpass on the rebased head.Routes checked:
Close from the phone, close from the desktop while the phone shows the tab, and a terminal process exit all reach the same per-device projection.
Headless
orca servepublishes tab groups and uses the same store.Folder workspaces: the selection is keyed by workspace id and needs nothing from git.
Phone app restart: the host keeps the selection, so the phone comes back to the same tab.
Old phone with a new host: fixed, because the host picks the tab.
New phone with an old host: the old leftmost behaviour stays. No new fields go over the wire.
Snapshots with no tab groups still fall back to the first tab.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Agent skill upstream boundary
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)