Repository navigation
Fix restored mobile terminals and workspace visibility parity - #8789
Conversation
Co-authored-by: Orca <help@stably.ai>
Co-authored-by: Orca <help@stably.ai>
Co-authored-by: Orca <help@stably.ai>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes add cutover-aware mobile terminal and session-tab activation helpers, mirrored tab-selection confirmation, and structured development diagnostics for terminal streams, tab fetches, WebViews, and route state. Account subscription refreshes now use stale-aware rate-limit handling and prevent overlapping inactive-account fetches. Runtime behavior now falls back to provider terminal history, tracks host-sidebar activity from persisted session state and browser tabs, resolves PTY ownership through persisted bindings, and coordinates overlapping terminal WebView surface replacements. 🚥 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mobile/app/h/[hostId]/session/[worktreeId].tsx (1)
2992-2999: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAvoid the duplicate activation RPC
tab.idalready identifies the pending terminal surface here, soleafIdisn’t needed. The explicit call and the effect below both activate the same tab; keep one path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 04e07080-0e1c-4722-af30-4d3a854eae04
📒 Files selected for processing (16)
mobile/app/h/[hostId]/session/[worktreeId].tsxmobile/src/session/mobile-session-route-helpers.tsmobile/src/session/mobile-session-startup-source.test.tsmobile/src/session/mobile-session-tab-activation.test.tsmobile/src/session/mobile-session-tab-activation.tsmobile/src/session/mobile-terminal-diagnostics.test.tsmobile/src/session/mobile-terminal-diagnostics.tsmobile/src/session/session-tab-snapshot-gate.test.tsmobile/src/session/session-tab-snapshot-gate.tssrc/main/rate-limits/service.test.tssrc/main/rate-limits/service.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/remote-runtime-request-connection.integration.test.tssrc/main/runtime/rpc/methods/accounts.test.tssrc/main/runtime/rpc/methods/accounts.ts
| export function logMobileTerminalDiagnostic( | ||
| event: string, | ||
| details: MobileTerminalDiagnosticDetails = {} | ||
| ): void { | ||
| // Why: lifecycle diagnostics are intentionally available for HMR repros, | ||
| // but high-frequency WebView events must not add production log overhead. | ||
| if (typeof __DEV__ !== 'undefined' && !__DEV__) { | ||
| return | ||
| } | ||
| // Keep this structured and content-free so users can safely share a filtered log. | ||
| console.log(MOBILE_TERMINAL_DIAGNOSTIC_TAG, event, details) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Dev-only gate fails open when __DEV__ is undefined.
typeof __DEV__ !== 'undefined' && !__DEV__ only suppresses logging when __DEV__ is explicitly false; if __DEV__ is undefined in some execution context, this diagnostic module logs unconditionally — the opposite of "development-only." Metro always defines __DEV__ in the shipped app, so the shipped-app risk is low, but the guard should fail closed to actually guarantee the "privacy-safe, development-only" property stated for this feature.
🛡️ Proposed fix (fail closed instead of fail open)
- if (typeof __DEV__ !== 'undefined' && !__DEV__) {
- return
- }
+ if (typeof __DEV__ === 'undefined' || !__DEV__) {
+ return
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function logMobileTerminalDiagnostic( | |
| event: string, | |
| details: MobileTerminalDiagnosticDetails = {} | |
| ): void { | |
| // Why: lifecycle diagnostics are intentionally available for HMR repros, | |
| // but high-frequency WebView events must not add production log overhead. | |
| if (typeof __DEV__ !== 'undefined' && !__DEV__) { | |
| return | |
| } | |
| // Keep this structured and content-free so users can safely share a filtered log. | |
| console.log(MOBILE_TERMINAL_DIAGNOSTIC_TAG, event, details) | |
| } | |
| export function logMobileTerminalDiagnostic( | |
| event: string, | |
| details: MobileTerminalDiagnosticDetails = {} | |
| ): void { | |
| // Why: lifecycle diagnostics are intentionally available for HMR repros, | |
| // but high-frequency WebView events must not add production log overhead. | |
| if (typeof __DEV__ === 'undefined' || !__DEV__) { | |
| return | |
| } | |
| // Keep this structured and content-free so users can safely share a filtered log. | |
| console.log(MOBILE_TERMINAL_DIAGNOSTIC_TAG, event, details) | |
| } |
| for (const tab of tabs) { | ||
| bind(tab.ptyId, worktreeId) | ||
| bind(session?.remoteSessionIdsByTabId?.[tab.id], worktreeId) | ||
| const layout = session?.terminalLayoutsByTabId[tab.id] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg "session(\?)?\.terminalLayoutsByTabId\[" src/main/runtime/orca-runtime.tsRepository: stablyai/orca
Length of output: 284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show all terminalLayoutsByTabId uses with line numbers and nearby context.
rg -n -C 3 "terminalLayoutsByTabId" src/main/runtime/orca-runtime.tsRepository: stablyai/orca
Length of output: 4488
Guard terminalLayoutsByTabId before indexing
Older persisted sessions can omit that map, so session?.terminalLayoutsByTabId[tab.id] can still throw. Use session?.terminalLayoutsByTabId?.[tab.id] here, and apply the same guard to the other direct access in this file.
Co-authored-by: Orca <help@stably.ai>
…ix) before landing host-endpoint-edit fixes
Two independent review rounds found the same class of foot-gun: a superseded mutator (updateHostEndpoint, then renameHost) left in host-store.ts after the atomic updateHostNameAndEndpoint refactor, with zero remaining callers. Either could be reintroduced by a future caller and silently regress the non-atomic name/endpoint race the atomic function was written to close, so both are removed. Also covers reconnect-rejection and endpoint-only save paths that were missing test coverage, and merges origin/main (stablyai#8789) so this lands without reverting the mobile terminal restore fix. Co-authored-by: Orca <help@stably.ai>
Reconcile mobile session route ([worktreeId].tsx) with stablyai#8789 (restored-terminal + workspace-visibility parity): keep the branch's native-chat coverage gates and input-lease wiring while adopting main's terminal-diagnostics hooks (terminalUnsubscribed, firstStreamEvent) and granular subscribe-skip logging.
* feat(mobile): edit saved host endpoints * fix(mobile): reject ambiguous numeric host addresses * fix(mobile): label edit host inputs * fix(mobile): make host edit save atomic and remove superseded mutators Two independent review rounds found the same class of foot-gun: a superseded mutator (updateHostEndpoint, then renameHost) left in host-store.ts after the atomic updateHostNameAndEndpoint refactor, with zero remaining callers. Either could be reintroduced by a future caller and silently regress the non-atomic name/endpoint race the atomic function was written to close, so both are removed. Also covers reconnect-rejection and endpoint-only save paths that were missing test coverage, and merges origin/main (#8789) so this lands without reverting the mobile terminal restore fix. Co-authored-by: Orca <help@stably.ai> * Simplify save-race comment and reword host-removed error message - Trims the redundant comment explaining the savingRef race guard down to one line. - Changes the "no longer saved" load-error copy to "was removed" for clearer phrasing, updating the matching test expectation. --------- Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Co-authored-by: Orca <help@stably.ai>
…ai#8789) * Fix mobile cutover activation and usage refresh loops Co-authored-by: Orca <help@stably.ai> * Fix restored mobile terminal state parity Co-authored-by: Orca <help@stably.ai> * Fix migrated PTY workspace attribution Co-authored-by: Orca <help@stably.ai> * Fix overlapping mobile terminal surface swaps Co-authored-by: Orca <help@stably.ai> --------- Co-authored-by: Orca <help@stably.ai>
* feat(mobile): edit saved host endpoints * fix(mobile): reject ambiguous numeric host addresses * fix(mobile): label edit host inputs * fix(mobile): make host edit save atomic and remove superseded mutators Two independent review rounds found the same class of foot-gun: a superseded mutator (updateHostEndpoint, then renameHost) left in host-store.ts after the atomic updateHostNameAndEndpoint refactor, with zero remaining callers. Either could be reintroduced by a future caller and silently regress the non-atomic name/endpoint race the atomic function was written to close, so both are removed. Also covers reconnect-rejection and endpoint-only save paths that were missing test coverage, and merges origin/main (stablyai#8789) so this lands without reverting the mobile terminal restore fix. Co-authored-by: Orca <help@stably.ai> * Simplify save-race comment and reword host-removed error message - Trims the redundant comment explaining the savingRef race guard down to one line. - Changes the "no longer saved" load-error copy to "was removed" for clearer phrasing, updating the matching test expectation. --------- Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Co-authored-by: Orca <help@stably.ai>
Summary
worktree.psclassify deferred startup workspaces, migrated/persisted legacy PTY bindings, provider-confirmed PTYs, and open-browser workspaces consistently with the desktop sidebarSupersedes #8781.
Fixes #8788.
Related: #7400 has a similar missing-terminal symptom, but its stale structural
mobileSessionTabsversioning root cause is separate and is not fixed by this PR.Root causes
A parked desktop terminal could register a renderer serializer before xterm restored its buffer. The runtime treated that empty renderer snapshot as authoritative and never consulted retained provider history, so Mobile received a valid but empty terminal snapshot.
Mobile also cleared its pending local tab selection when it received a
mobile-local:publication. That publication acknowledged Mobile's own state, not the mirrored desktop focus, allowing the next desktop snapshot to snap selection back.For the workspace list, Mobile intentionally filters sleeping workspaces like desktop, but the runtime activity projection did not include deferred
activeWorktreeIdsOnShutdown, browser-only activity, or legacy live PTYs whose persisted tab/layout binding was the only reliable worktree attribution. Persisted ownership also has to win after a workspace identity migration because a running daemon PTY keeps the old worktree id minted into its session id. Clicking a workspace on desktop mounted its pane and made the row appear, masking these projection gaps.Separately, authenticated connection migration rejects in-flight requests by design, while account subscription replay used the same forced refresh lane as an explicit user refresh. Those behaviors caused dropped activation requests and repeated usage fetch cycles.
Screenshots
No visual change.
Testing
pnpm lintpnpm typecheckpnpm test— full root suite not run; focused runtime/account/usage coverage is listed belowpnpm build— no build-system or dependency changesAdditional validation:
git diff --checkAI Review Report
An independent regression review checked the final diff's terminal snapshot authority, output sequencing, pending mobile/desktop selection gates, diagnostic lifecycle, persisted PTY ownership, direct/relay cutover, SSH/runtime projection, and older-host behavior.
An earlier provider-preference design raised output-sequencing and serialization-cost concerns; the final implementation avoids that design and only falls back when the renderer snapshot is empty and retained provider content exists. The final review then found one workspace migration edge: persisted layout ownership was evaluated after the old worktree id frozen in a daemon PTY id. Follow-up commit
eb3c2f64areverses that precedence and adds the renamed-workspace regression test.Cross-platform review found no new shortcuts, labels, shell commands, or platform-specific path assumptions. The ownership logic operates on persisted runtime ids and keeps existing native, WSL, SSH, and relay resolution paths intact.
Security Audit
No security follow-up is required.
Notes
Real-device verification:
The terminal-history and workspace-projection fixes require the desktop changes. Tab-selection handling requires the Mobile changes. #7400 remains separate follow-up work.