Repository navigation
Distinguish remote browser failures from server connection failures - #26573
Conversation
0ae1d0c to
725d1f8
Compare
725d1f8 to
a90426e
Compare
📝 WalkthroughWalkthroughThe remote browser stream uses a shared failure resolver for opening and restart phases. It displays a service-specific notice for Priority: ➖ Normal Merge Risk: 🔵 Low · up to Retry exhaustion can show a misleading connection-loss message, and the new E2E test may miss an incorrect result after Reconnect. Both are bounded issues that should be fixed or explicitly accepted before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 10 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 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
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
7b650c05-7261-4756-98d8-924de8fbc5e3
📒 Files selected for processing (12)
.github/workflows/e2e.ymlconfig/scripts/ci-e2e-job-selection.mjsconfig/scripts/ci-e2e-job-selection.test.mjsconfig/scripts/pr-e2e-source-routing.mjssrc/renderer/src/components/browser-pane/stream-remote/remote-browser-service-failure.test.tssrc/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-errors.test.tssrc/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-errors.tssrc/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-lifecycle.tssrc/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-restart-attempt.tssrc/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-status.tssrc/renderer/src/i18n/locales/en.jsontests/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; 7 remain after this review.
| deps.setStatus(remoteBrowserStreamStopped(remoteBrowserStreamLostNotice())) | ||
| this.setStatus( | ||
| remoteBrowserStreamStopped( | ||
| remoteBrowserStreamNotice(this.status) ?? remoteBrowserStreamLostNotice() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Retain the classified notice until the stream is ready.
If a retry reports browser_unavailable and a later subscription closes before ready, handleStreamClosed sets the status to retrying(null). When the retry budget then expires, this expression selects the connection-loss notice instead of the last classified service notice. Store the last classified notice separately from the current status. Clear it on a fresh open or a ready stream.
| await notice.getByRole('button', { name: 'Reconnect', exact: true }).click() | ||
| await expect(notice).toContainText( | ||
| 'The remote browser is unavailable. Check its setup on the server.' | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
set -eu
printf '%s\n' '--- changed file with line numbers ---'
nl -ba tests/e2e/ssh-orcad-browser-service-status.spec.ts | sed -n '1,240p'
printf '%s\n' '--- diff against merge base ---'
git diff --find-renames 1cda323b6ad9b51847a5a76e49f11357ba700ced a90426e27414f06b4d079b3cfe3cb9cce56b9fdf -- tests/e2e/ssh-orcad-browser-service-status.spec.ts
printf '%s\n' '--- reconnect references in test sources ---'
rg -n -F --glob 'tests/e2e/**/*.{ts,tsx}' -- 'getByRole('\''button'\'', { name: '\''Reconnect'\'', exact: true })' tests/e2e || test "$?" -eq 1
printf '%s\n' '--- relevant notice/reconnect source references ---'
rg -n -F --glob '*.{ts,tsx}' -- 'The remote browser is unavailable.' . || test "$?" -eq 1
rg -n -F --glob '*.{ts,tsx}' -- 'Cannot reach the remote server.' . || test "$?" -eq 1Repository: stablyai/orca
Length of output: 18815
🏁 Script executed:
set -eu
printf '%s\n' '--- paired reconnect test around both reconnect flows ---'
nl -ba tests/e2e/paired-remote-browser-stream-reconnect.spec.ts | sed -n '60,115p;195,245p'
printf '%s\n' '--- stream status and lifecycle source ---'
nl -ba src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-status.ts | sed -n '1,150p'
nl -ba src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-lifecycle.ts | sed -n '1,260p'
printf '%s\n' '--- reconnect and error-notice implementation references ---'
rg -n -F --glob 'src/**/*.{ts,tsx}' -- 'remote-browser-stream-error' src || test "$?" -eq 1
rg -n -F --glob 'src/**/*.{ts,tsx}' -- 'Reconnect' src/renderer/src/components/browser-pane/stream-remote src || test "$?" -eq 1Repository: stablyai/orca
Length of output: 42339
🏁 Script executed:
set -eu
printf '%s\n' '--- exact error notice rendering ---'
rg -n -F --glob 'src/renderer/src/components/browser-pane/**/*.{ts,tsx}' -- 'remote-browser-stream-error' src/renderer/src/components/browser-pane
rg -n -F --glob 'src/renderer/src/components/browser-pane/**/*.{ts,tsx}' -- 'canReconnectRemoteBrowserStream' src/renderer/src/components/browser-pane
rg -n -F --glob 'src/renderer/src/components/browser-pane/**/*.{ts,tsx}' -- 'streamStatus' src/renderer/src/components/browser-pane
printf '%s\n' '--- relevant viewport and pane blocks ---'
nl -ba src/renderer/src/components/browser-pane/stream-remote/remote-browser-page-viewport.tsx | sed -n '1,220p'
nl -ba src/renderer/src/components/browser-pane/stream-remote/remote-browser-page-pane.tsx | sed -n '1,240p'
printf '%s\n' '--- remaining lifecycle open/reconnect block ---'
nl -ba src/renderer/src/components/browser-pane/stream-remote/remote-browser-stream-lifecycle.ts | sed -n '240,520p'Repository: stablyai/orca
Length of output: 35109
Make the post-retry assertion wait for the retry outcome.
The notice already contains the unavailable text before the click. The assertion after the click can pass before the retry runs.
🐛 Suggested fix
--- "a/tests/e2e/ssh-orcad-browser-service-status.spec.ts"
+++ "b/tests/e2e/ssh-orcad-browser-service-status.spec.ts"
@@ -153,10 +153,13 @@
'The remote browser is unavailable. Check its setup on the server.'
)
await expect(notice).not.toContainText('Cannot reach the remote server.')
await notice.getByRole('button', { name: 'Reconnect', exact: true }).click()
+ await expect(notice).toBeHidden({ timeout: 30_000 })
+ await expect(notice).toBeVisible({ timeout: 60_000 })
await expect(notice).toContainText(
'The remote browser is unavailable. Check its setup on the server.'
)
+ await expect(notice).not.toContainText('Cannot reach the remote server.')
console.log('[browser-service-after-retry]', await notice.innerText())
expect(JSON.parse(await serverCall(page, environment.id, 'repo.list'))).toMatchObject({
ok: true,📝 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.
| await notice.getByRole('button', { name: 'Reconnect', exact: true }).click() | |
| await expect(notice).toContainText( | |
| 'The remote browser is unavailable. Check its setup on the server.' | |
| ) | |
| await notice.getByRole('button', { name: 'Reconnect', exact: true }).click() | |
| await expect(notice).toBeHidden({ timeout: 30_000 }) | |
| await expect(notice).toBeVisible({ timeout: 60_000 }) | |
| await expect(notice).toContainText( | |
| 'The remote browser is unavailable. Check its setup on the server.' | |
| ) | |
| await expect(notice).not.toContainText('Cannot reach the remote server.') |
2 participants
ELI5
A Remote host could answer repository requests while its browser was unavailable. Orca still said it could not reach the server. It now identifies the browser setup failure and keeps a way to try again.
What Changed
The existing browser failure classifier recognizes the host’s
browser_unavailablereply separately from a transport failure. Initial opening, stream recovery, and resizing use that classifier. When the retry budget expires, the lifecycle preserves its last classified notice instead of replacing it with a connection-loss message.Manual recovery and the existing bounded automatic retries remain available. The real managed SSH test verifies a browser refusal alongside successful repository calls, checks the rendered notice, and retries through the visible control. CI runs it in the managed-server template lane.
Why
A browser service failure is not evidence that the execution host is unreachable. One classifier keeps all stream failure paths consistent, while preserving recovery because the browser backend can return without the server connection changing.
Linked Issue
Follow-up to the SSH consolidation in #26483. Independently based on OrcaWin/phase35.
Visual Proof
Before: a responding host’s browser failure is reported as a server outage.
After: the notice identifies the unavailable browser, with recovery still offered.
Baseline failure · Browser refusal, responsive host, corrected notice, and manual retry
Testing
172 tests pass across 15 browser stream and CI contract files. They cover initial failure, resize, recovery when the backend returns, retry exhaustion, real transport failures, and the existing lifecycle behavior.
Full
pnpm tc, changed-code quality, affected oxlint, localization catalog/extraction/coverage checks, commit checks, and a fresh Electron E2E build pass. On macOS with an owned Linux SSH host, the real baseline fails the notice assertion and two corrected runs pass, including manual retry. Windows validation is left to CI.Review
Self-reviewed failure-code provenance, stream supersession, retry-budget behavior, notice clearing on recovery, localization, and mixed-server compatibility.
Agent skill upstream boundary
Notes
This fixes failure reporting, not browser availability or placement. #26570 and #26572 address the separate conversion routing and retained-page placement bugs. No client/server wire change or UI restyling is required.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)