Skip to content

Preserve retained desktop browser pages during SSH conversion - #26572

Merged
OrcaWin merged 4 commits into
mainfrom
nwparker/ssh-retained-browser-placement
Oct 9, 2026
Merged

OrcaWin merged 4 commits into
mainfrom
nwparker/ssh-retained-browser-placement

Conversation

@nwparker

@nwparker nwparker commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​254 $\color{#cf222e}{\Huge{\mathbf{−}}}$​22 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​232
Prod 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​62 $\color{#cf222e}{\Huge{\mathbf{−}}}$​3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​59

ELI5

An SSH workspace could keep its browser tab after moving to Remote, yet opening that tab showed a connection error. The app had changed where it expected the browser to run. Existing desktop pages now stay on the desktop and keep their login through conversion, reconnect, and app restart.

What Changed

Before refreshing a converted host’s catalogs, the renderer records explicit desktop placement for its existing browser pages. It uses the current workspace owner to include both git and folder workspaces, leaving other hosts and explicit remote pages alone. URL history across document conversion keeps the same placement. Future normal and profile tabs also record the local provider choice at creation.

The real browser routing journey now checks the original tab as well as a new tab, then reconnects SSH and restarts the app. Source changes select that journey in the managed-server CI lane.

Why

Unspecified placement inherits the workspace owner. That owner changes during conversion, although a desktop browser has not moved to the server. Recording the existing placement before the change preserves its meaning and avoids attempting unsupported server streaming or creating a replacement browser on another host.

Linked Issue

Follow-up to #26483. Stacked on #26570: that PR preserves the SSH route for desktop pages once their workspace has managed ownership. This PR fixes the existing page’s placement; merge #26570 first.

Visual Proof

Before: the retained tab attempts a server browser and shows a connection error.

Before

After: the original tab reaches the remote-only website with its original login cookie.

After

Baseline failure · Conversion, reconnect, restored placement, and restart request records

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

132 tests pass across 11 affected browser, conversion, and CI contract files. Full pnpm tc, affected oxlint, changed-code quality, commit checks, and a fresh Electron E2E build pass.

Real hidden Electron on macOS with an owned Linux SSH host fails the retained-page assertion before the fix. Corrected runs reach the private origin with the same partition and login. The final journey also verifies both pages after SSH reconnect and the retained page after a full app restart. A separate saved relay-era browser startup journey passes automatic conversion, reconnect, and another app restart. An owned Linux SSH folder-workspace journey also passes conversion, new-page creation, reconnect, and full app restart; folder ownership is additionally covered by unit tests. Windows validation is left to CI.

The optional repository-wide E2E typecheck reports 246 diagnostics in existing other files; none names a changed file. The standard full typecheck passes.

Review

Self-reviewed conversion ordering, ownership boundaries, explicit remote placement, document history, persistence, and no-op identity retention.

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

Uses the existing optional placement field; no paired client/server protocol changes. Login survival is verified from the origin’s requests. This does not promise preservation of in-page memory or browser history across app restart.

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)

@OrcaWin
OrcaWin force-pushed the nwparker/ssh-browser-route-after-conversion branch from 26bcbff to 890075a Compare October 9, 2026 02:12
@OrcaWin
OrcaWin requested a review from brennanb2025 as a code owner October 9, 2026 02:12
@OrcaWin
OrcaWin force-pushed the nwparker/ssh-retained-browser-placement branch from b067bac to 7680e04 Compare October 9, 2026 02:12
@OrcaWin
OrcaWin force-pushed the nwparker/ssh-browser-route-after-conversion branch 2 times, most recently from d1586c1 to ae8c3b5 Compare October 9, 2026 04:11
@OrcaWin
OrcaWin changed the base branch from nwparker/ssh-browser-route-after-conversion to main October 9, 2026 04:30
@OrcaWin
OrcaWin force-pushed the nwparker/ssh-retained-browser-placement branch from 7680e04 to d9793b3 Compare October 9, 2026 04:30
@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8ccb660a-d65f-4b7e-8404-a260aa69f8d2

📥 Commits

Reviewing files that changed from the base of the PR and between 339a285 and 941e7577ac87e088233bcfc1fad1c9343e892334.


📒 Files selected for processing (1)
  • tests/e2e/ssh-orcad-browser-service-status.spec.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

Walkthrough

The change adds explicit local placement for converted SSH browser pages and URL conversion origins. Managed-server catalog loading preserves that placement, and browser-tab creation fallbacks pass a null runtime environment ID. Tests cover page preservation, managed conversion, and SSH-backed tab creation. E2E tests check retained-tab authentication across managed conversion, reconnection, and app restart, and verify that a new managed-runtime tab differs from the retained tab. E2E source routing and CI selection include the affected renderer paths.


Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 941e7

The change preserves converted browser pages while keeping new managed-runtime tabs distinct. No actionable merge risk remains in the reviewed evidence; it is ready to merge subject to normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 10 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: preserving retained desktop browser pages during SSH conversion.
Description check Passed The description substantially follows the template. It explains the user impact, implementation, rationale, linked work, visual proof, testing, review notes, compatibility considerations, and checklis…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.


✨ 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


  • Autopilot · 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.

@OrcaWin
OrcaWin force-pushed the nwparker/ssh-retained-browser-placement branch 3 times, most recently from 941e757 to efd0154 Compare October 9, 2026 08:49
nwparker and others added 3 commits October 9, 2026 02:24
Converted SSH workspaces now keep retained desktop pages local, so the
spec opens its remote-placed page on the managed environment instead of
relying on the retained page moving there.
…nect

The managed host can seed a terminal for the converted workspace and the
client mirrors it with focus, hiding the browser notice mid-test.
@OrcaWin
OrcaWin force-pushed the nwparker/ssh-retained-browser-placement branch from efd0154 to 42c47ac Compare October 9, 2026 09:26
@OrcaWin
OrcaWin merged commit c12c211 into main Oct 9, 2026
39 checks passed
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.

2 participants