Skip to content

ci(e2e): run the SSH browser route e2e when its source changes - #23498

Merged
brennanb2025 merged 1 commit into
mainfrom
brennanb2025/route-ssh-browser-e2e
Sep 28, 2026
Merged

brennanb2025 merged 1 commit into
mainfrom
brennanb2025/route-ssh-browser-e2e

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​22 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​21
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​11 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​11

ELI5

CI decides which end-to-end tests a PR runs by looking at the files it changes. Changes to the code that routes browser tabs through an SSH host selected no end-to-end tests at all, so the one test that exercises that code never ran on the PR that broke it. Now a change to those files runs that test.

What Changed

Before: #22810 changed use-ssh-workspace-browser-route.ts and the host-connection signal it waits on. CI's routing script mapped those files to zero specs, so the whole e2e workflow was skipped. local-ssh-browser-routing.spec.ts first ran on the nightly main run after merge, and it has failed on most main-based PRs since then.

After: a new route, browser.local-ssh-workspace-route, in config/scripts/pr-e2e-source-routing.mjs sends these files to tests/e2e/local-ssh-browser-routing.spec.ts:

  • src/main/browser/local-ssh-browser-route.ts and local-ssh-browser-partitions.ts
  • src/renderer/src/components/browser-pane/use-ssh-workspace-browser-route.ts
  • src/renderer/src/components/browser-pane/assemble-chrome/ssh-routed-browser-page-gate.tsx (the "SSH connection unavailable" card)
  • src/renderer/src/lib/worktree-host-connection-phase.ts (the host-connection phase the route gate waits on)

Mechanism: no workflow change was needed. The SSH Docker e2e job already runs when this spec is among the selected specs, and the general changed-specs job already skips it. Only the file-to-spec route was missing.

Why

This adds one route beside the existing ones, following the same pattern. Alternatives considered:

  • Adding these paths to the ssh-terminal-source route would run twelve SSH terminal specs for a browser change.
  • Routing all of browser-pane/** would put the SSH Docker lane on most browser PRs, even though most browser-pane files never touch SSH.

The host-connection phase is included because a change there can strand the SSH-unavailable card without touching any browser file, which is what #22810 did.

Linked Issue

Follow-up to #22810. The test failure itself is fixed separately in #23465.

Visual Proof

N/A. CI routing only; nothing changes in the app.

Testing

  • Replayed fix(workspace): a restored workspace paints before its terminals reconnect #22810's changed files through the routing script. On main: no specs, e2e skipped. On this branch: local-ssh-browser-routing.spec.ts is selected and e2e runs.
  • Added a case to config/scripts/ssh-browser-e2e-routing.test.mjs. It covers every routed file and checks that tests beside them and an unrelated browser-pane file do not select the spec. It fails with the route removed and passes with it.
  • Routing and lane contract tests pass: ssh-browser-e2e-routing, pr-e2e-*, ssh-localhost-e2e-routing, and wsl-e2e-lane-contract (5 + 2 files, 110 tests).
  • check:code-quality:changed, oxlint, and oxfmt are clean.

This PR changes only config/scripts/, which no route matches, so its own CI will not run the SSH lane. Until #23465 merges, a PR that touches the routed files will see this spec fail, as it does on main today.

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

No app runtime change, so it has no effect on cross-platform, SSH, mobile, or compatibility behavior. PRs that touch the five routed files now also run the SSH Docker e2e lane.

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)

A change to the SSH workspace browser route, its gate card, or the host-connection
phase it waits on selected no e2e specs, so the Docker SSH browser spec never ran
on the PR that changed it.
@coderabbitai

coderabbitai Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

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: 087d70d7-0307-4a4b-ac7f-5b3d2973c685

📥 Commits

Reviewing files that changed from the base of the PR and between 89cf55d and a230e91.

📒 Files selected for processing (2)
  • config/scripts/pr-e2e-source-routing.mjs
  • config/scripts/ssh-browser-e2e-routing.test.mjs

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


📝 Walkthrough

Walkthrough

The PR adds the browser.local-ssh-workspace-route source route for local SSH browser routing changes. The route selects tests/e2e/local-ssh-browser-routing.spec.ts. Tests cover matching source paths, reusable PR E2E eligibility, and unrelated changes.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to a230e

The SSH browser E2E routing change is ready to merge after normal checks.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely states that CI will run the SSH browser route end-to-end test when related source files change.
Description check ✅ Passed The description is complete and focused. It explains the problem, changed files, mechanism, rationale, linked issue, testing, scope, and compatibility impact. The full project validation checklist rem…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
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
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR

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.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review status for a230e917f8cdcd893a8a205f01fc1dc8789081bf: clean within this CI-routing scope. An independent Codex reviewer completed one review pass with no actionable findings. Initial and final readiness audits found no proven P0/P1/P2 issues. No code changes were warranted, and no additional commits were pushed.

  • Correctness: the missing source-to-test mapping let relevant source changes skip E2E. The new rule adds that mapping at its existing owner. Comparing all 30,186 tracked paths against current origin/main changes selection for exactly the five intended files; each adds only local-ssh-browser-routing.spec.ts. The existing workflow selects its Docker owner and enables the required test flag.
  • Architecture and scope: this extends the centralized source-to-spec table. Spec selection then activates the existing reusable workflow and Docker lane; there is no duplicate workflow path list or new runtime state. That lane runs its full four-shard suite, not just this browser spec. Its existing advisory/nonblocking policy is unchanged.
  • Performance and compatibility: one bounded CI path matcher; no app hot-path work, polling, persistence, dependency, mobile-wire, or platform-runtime changes. The existing Docker suite is the additional CI cost on those source changes; wall-clock cost was not benchmarked.
  • Validation: 68 tests across six routing/workflow contract files passed, independently repeated by the reviewer. Changed-code quality, oxlint, formatting, and diff checks passed.
  • Remaining gaps: hosted checks are still pending. Docker/Electron and remote-machine journeys were intentionally not rerun for this routing-only change. This does not fix the underlying browser failure; fix(browser): a failed SSH route keeps its card while the host redials #23465 remains the separate open fix.

Review complete; no merge performed.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

The PR is a CI routing fix with no app runtime change. I verified the mechanism end to end at a230e91.

  • New route browser.local-ssh-workspace-route — config/scripts/pr-e2e-source-routing.mjs maps the five SSH browser-route source files to tests/e2e/local-ssh-browser-routing.spec.ts. The regex is fully anchored and isProductSource excludes the .test.ts/.test.tsx siblings, so the route matches exactly those files.
  • Lane actually runs — selectPrE2eSpecs selects the spec, shouldRunReusablePrE2e returns true, and e2e.yml already excludes the spec from changed-e2e while listing it in the ssh-docker-watcher-isolation if. run-ssh-docker-e2e.mjs already claims it with ORCA_E2E_SSH_DOCKER/ORCA_E2E_LOCAL_SSH_BROWSER, so the Docker-SSH lane executes it.
  • Regression case covered — ssh-browser-e2e-routing.test.mjs adds positive assertions for all five files (including shouldRunReusablePrE2e) and negative assertions for adjacent unit tests and BrowserPane.tsx. Removing the route fails it.
  • Fix targets the right seam — worktree-host-connection-phase.ts was created by #22810; probing the route against that commit's files confirms both use-ssh-workspace-browser-route.ts and the phase module now select the spec.

I ran vitest config/scripts/ssh-browser-e2e-routing.test.mjs config/scripts/pr-e2e-gate-contract.test.mjs — 45 passed.

ℹ️ Broad trigger on worktree-host-connection-phase.ts

worktree-host-connection-phase.ts is also imported by editor-panel content loading (useEditorPanelFileContentLoader.ts, useEditorPanelFileLoadRetry.ts) and terminal pane host state, so an edit to it now runs the 4-shard / 60-minute Docker-SSH lane even for PRs unrelated to SSH routing. The route comment and PR body justify this, and there is no narrower seam for the phase signal, so this reads as a deliberate tradeoff rather than a defect — noting it only so the cost is visible if that lane's runtime becomes a problem.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025
brennanb2025 merged commit d6336be into main Sep 28, 2026
34 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.

1 participant