Fix frontend-only backend setup and CORS handling#951
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
c797687 to
66db084
Compare
- avoid seeding a backend for frontend-only dev launches - route missing or unauthorized agent server preflight to backend settings - surface CORS failures with localized agent-server launch guidance - keep create-conversation CORS errors to a single toast
66db084 to
9d7ca75
Compare
|
@OpenHands /iterate |
|
Uh oh! There was an unexpected error starting the job :( |
|
@OpenHands /iterate |
|
Uh oh! There was an unexpected error starting the job :( |
|
@OpenHands /iterate |
|
I'm on it! neubig can track my progress at all-hands.dev |
….tsx conflict Conflict in src/root.tsx: PR replaces MissingAgentServerScreen (modal-based) with a redirect to /settings/backend + AgentServerConnectionScreen; main added links() export for the favicon. Resolution: keep PR's redirect logic, preserve links() export. Co-authored-by: openhands <openhands@all-hands.dev>
The PR changed readStoredBackends() to only seed the default local backend when hasConfiguredAgentServerDefaults() is true (deployed with env vars). In Vitest environments without VITE_BACKEND_BASE_URL, the function returns false so no seeding occurs. - active-backend-context.test.tsx: mock hasConfiguredAgentServerDefaults to return true so seeding tests still exercise the production path - agent-server-compatibility-bundled-pin.test.ts: same mock so hasEffectiveLocalBackend() returns true for the no-local-backends case - add-backend-modal.test.tsx: change toHaveLength(2) → toHaveLength(1) since without env defaults no default backend is seeded before the test Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed CI failures in 649bc8a. Two commits were pushed:
This comment was created by an AI agent (OpenHands) on behalf of the PR author. |
Summary of Work on PR #951What was requestedThe What was done✅ Identified the problemThe PR branch ✅ Resolved the merge conflict
✅ Fixed CI test failures (3 tests in 2 CI jobs)The PR had changed
✅ Final CI state
⏳ Remaining blockerThe PR is Changes are conciseAll three commits are directly required to address the PR's state: one merge commit (necessary to resolve conflicts) and one test-fix commit (necessary to make CI pass). No extraneous changes were made. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
This is a well-structured PR that addresses three distinct but related pain points: the frontend-only dev workflow defaulting to a backend that may not exist, the absence of actionable CORS error guidance, and the "unauthorized" vs "unreachable" distinction for backend errors. The overall design is sound. A few issues are worth addressing.
✅ Strengths
- CORS detection + localized guidance — Surfacing
OH_ALLOW_CORS_ORIGINSinstructions with translated detail strings is excellent UX for the cross-origin deployment scenario. ThefrontendOrigin !== backendOriginguard prevents false positives for same-origin failures. AgentServerUnavailableReasondiscriminated union — Properly separates"unauthorized"(401/403) from"unreachable"(network/other HTTP) failure modes, enablingAgentServerConnectionScreento give context-specific guidance.preflightAgentServerAccesstwo-phase check — Probing/server_infofor reachability then/api/settingsfor auth is clean; distinguishes the two failure modes without a dedicated auth endpoint.Navigateto/settings/backendinstead of modal overlay — The connection screen gets its own URL, the modal-on-top-of-nothing pattern is gone, and thebackend-settings.tsxredirect-to-home on successful reconnect is an elegant post-recovery UX.- Query key scoped to backend identity —
WEB_CLIENT_CONFIG_BY_BACKEND(backend)prevents stale config from surviving a backend switch. - Stale auto-seeded backend cleanup — Removing the empty-
apiKeydefault-local entry when defaults are no longer configured avoids phantom backends after switching to frontend-only mode.
🔴 Issue 1 — Silent error swallowing in HomeChatLauncher
The onError callback now dismisses the loading toast without any user-facing feedback. Previously it called displayErrorToast for any error. Now, only CORS errors surfaced via the useConfig/AgentServerConnectionScreen path are visible — but that path only covers the initial config load, not conversation creation.
Any createConversation failure (network timeout, server 5xx, SDK error, non-CORS reasons) will silently dismiss the toast with no indication to the user. Consider at minimum showing a fallback for non-CORS errors, or add a comment explaining which path surfaces them.
🟡 Issue 2 — Inconsistent normalization in isAutoSeededDefaultLocalBackend
In src/api/backend-registry/storage.ts:
LEGACY_FRONTEND_ONLY_DEV_BACKEND_URL ("http://127.0.0.1:8000") is compared against an already-normalized host without itself being normalized. If normalizeHostForComparison strips the protocol prefix or trailing slashes, this branch will never match and the legacy cleanup becomes dead code.
Fix: host === normalizeHostForComparison(LEGACY_FRONTEND_ONLY_DEV_BACKEND_URL)
🟡 Issue 3 — English-only browser error strings for CORS detection
In src/utils/agent-server-cors-error.ts, FETCH_NETWORK_ERROR_FRAGMENTS contains English-language browser error messages. Browsers with non-English UI locales or future browser versions that change error wording will silently miss CORS detection. The detection degrades gracefully (no false positives given the frontendOrigin !== backendOrigin guard), but a brief comment explaining the heuristic and its known limitations would help maintainers.
🟢 Minor — Dead code in containsHtmlDocument regex
In src/api/agent-server-compatibility.ts, the regex includes <!doctype and <html (HTML-entity-encoded forms). SDK error messages come from raw HTTP response bodies and contain literal < characters, not <. These variants appear to be dead code; removing them simplifies the regex without changing behavior.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
@OpenHands /iterate |
|
Uh oh! There was an unexpected error starting the job :( |
|
@OpenHands /iterate |
|
I'm on it! neubig can track my progress at all-hands.dev |
e874ba6 to
1fac6c2
Compare
1fac6c2 to
d81e2e2
Compare
d81e2e2 to
72f550f
Compare
72f550f to
e977698
Compare
|
PR Artifacts Notice This PR contains a
|
…d-cors-flow # Conflicts: # __tests__/utils/mcp-marketplace-utils.test.ts
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Overview
This is a well-structured refactor with a clear goal: stop conflating "same-origin launcher-managed backend" with "any local backend", and stop baking launcher-specific config into static builds at compile time.
The key changes are:
kind: "local"→kind: "agent-server"across the codebase, with transparent migration of stored values on read vianormalizeBackendKind.AgentServerTransport("same-origin" | "remote") distinguishes whether the frontend and agent-server share an origin (static/package launchers) from a separately hosted backend added via the UI. This drivesgetBackendBaseUrl()to returnwindow.location.originfor same-origin backends, eliminating hardcoded host comparisons elsewhere.AgentCanvasRuntimeConfig/window.__AGENT_CANVAS_RUNTIME_CONFIG__replaces theopenhands-agent-server-configlocalStorage-write-on-load approach with a clean runtime injection point inindex.html, meaning session keys are no longer baked into the bundle at publish time.- CORS error utility surfaces an actionable message when a cross-origin network failure is likely a CORS misconfiguration, with origin-mismatch guard to prevent false positives.
getBackendBaseUrl()centralises URL resolution so the "same-origin →window.location.origin" rule can't drift out of sync across callers.- Implicit
/projectsworkspace parent removal: the dev-only heuristic that probed/projectswas causing noisy 404s against remote backends; good cleanup. - Fallback screen simplified:
MissingAgentServerScreennow rendersAddBackendModaldirectly (no lazyManageBackendsModal) withshowCloseButton=false, forcing users to configure a backend before proceeding. The new test cases covering 401/unauthorized flows are a solid addition.
The migration path (normalizeBackendKind, normalizeAgentServerTransport) handles old stored values cleanly, and coverage for the new auth-failure root states is thorough.
A few issues worth addressing are noted below.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| "failed to fetch", | ||
| "load failed", | ||
| "networkerror when attempting to fetch resource", | ||
| ] as const; |
There was a problem hiding this comment.
🟠 Important: browser error message matching is fragile across locales and browser versions
The three strings (, , ) are hardcoded English error messages from Chromium, Safari, and Firefox respectively. They will miss CORS failures in non-English browser locales (Firefox and Safari localise messages in some versions) and in future browser releases that change the wording.
All real CORS/network failures from surface as in every spec-compliant browser. Since the origin-mismatch guard at the call site already confirms the backend is cross-origin, checking first gives a much more robust signal:
Keeping the string fallback as a secondary check preserves behaviour for SDK-wrapped errors that strip the original identity.
| @@ -417,9 +437,12 @@ async function handleStatic(req, res, dirAbs, sessionApiKey = null) { | |||
| filePath = resolve(filePath, "index.html"); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
🟠 Important: property name collision is confusing
The local variable (built in ) has a property also named (the parsed JSON payload). requires double-reading and the same pattern appears a second time a few lines below, making both branches hard to scan.
Consider renaming the inner property when building the composite object in :
Then replace with throughout and . This also makes self-documenting.
| VITE_AGENT_SERVER_PROXY_TARGET: configuredProxyTarget, | ||
| VITE_USE_TLS = "false", | ||
| VITE_FRONTEND_PORT = "3001", | ||
| VITE_INSECURE_SKIP_VERIFY = "false", |
There was a problem hiding this comment.
🟡 **Suggestion: silently overrides all file values, not just **
The spread order means any shell/CI variable already set takes precedence over for every key destructured here, not just the new proxy target. This is almost certainly intentional to let pass without requiring a file, but it can silently shadow developer overrides for , , etc.
A narrower alternative is to only promote the new variable from :
If the current broad-override behaviour is intentional, a short comment explaining why would help future readers avoid inadvertently reverting it.
| if (typeof value !== "object" || value === null) return null; | ||
| const v = value as Partial<Backend>; | ||
| return ( | ||
| const rawKind = (value as { kind?: unknown }).kind; |
There was a problem hiding this comment.
🟡 Suggestion: migration values / should be documented or removed once fully migrated
maps legacy values ( → , → ). If these are one-time migration aliases for values that were written by a previous version, a brief comment noting which release introduced and which release can remove them would help avoid keeping dead migration code indefinitely. If they are still being written somewhere, that write site should be updated to use the canonical values.
|
Actually we have a several-step onboarding process already (select agent, backend, llm, starting prompt), so I don't think that we need to add the initial screen for onboarding a backend. OTOH, we need to make sure that the onboarding process works regardless of they way of launching. Fix this PR to rely on the onboarding process instead of adding an extra screen. |
|
Uh oh! There was an unexpected error starting the job :( |
📸 Snapshot Test ReportWarning Snapshot comparison step crashed (timeout, OOM, or runner error) — diff results below may be incomplete or absent. ✅ 4 snapshots changed — acknowledged via the
🔴 Changed snapshots (4)
|
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
![]() |
![]() |
![]() |
backend-manage-two-listed
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
![]() |
![]() |
![]() |
backends
backend-manage-modal
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
![]() |
![]() |
![]() |
onboarding
onboarding-step-1-check-backend
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
![]() |
![]() |
![]() |
✅ Unchanged snapshots (69)
archived-conversation
- conversation-panel-with-archived-badges
- conversation-view-archived
- conversation-view-sandbox-error
automations
- automations-delete-modal
- automations-list-active-inactive
- automations-no-automations
- automations-search-no-results
backends-extended
- backend-add-blank-disabled
- backend-add-cloud-advanced-open
- backend-add-cloud-no-key-disabled
- backend-add-cloud-with-key-enabled
- backend-add-form-partially-filled
- backend-add-invalid-url-disabled
- backend-add-local-ready
- backend-add-name-only-disabled
- backend-add-two-column-layout
- backend-add-whitespace-host-disabled
- backend-after-switch
- backend-dropdown-two-backends
- backend-edit-prefilled
- backend-manage-after-removal
- backend-remove-cancelled
- backend-remove-confirmation
- backend-switch-overlay
backends
- backend-add-modal
- backend-selector-open
changes-tab
- changes-deleted-file
- changes-diff-viewer
- changes-empty
collapsible-thinking
- reasoning-content-collapsed
- reasoning-content-expanded
- think-action-collapsed
- think-action-expanded
mcp-page
- mcp-custom-server-1-editor-open
- mcp-custom-server-2-url-filled
- mcp-custom-server-3-all-filled
- mcp-custom-server-4-installed
- mcp-custom-server-editor
- mcp-empty-installed
- mcp-search-filtered
- mcp-slack-install-1-marketplace
- mcp-slack-install-2-modal
- mcp-slack-install-3-filled
- mcp-slack-install-4-installed
onboarding
- onboarding-step-0-choose-agent
- onboarding-step-2-setup-llm
- onboarding-step-3-say-hello
projects-workspace-browser
- projects-workspace-browser
settings-page
- add-backend-modal
- analytics-consent-modal
- home-screen
- settings-app-page
- settings-page
settings-secrets
- secrets-add-form-filled
- secrets-add-form
- secrets-after-save
- secrets-delete-confirm
- secrets-list
settings-verification
- condenser-settings
- verification-settings-off
- verification-settings-on
sidebar
- sidebar-collapsed
- sidebar-conversation-panel
- sidebar-filter-menu
skills-page
- skills-empty
- skills-loaded
- skills-no-match
- skills-search-filtered
- skills-type-filter
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers.












Summary
kind: "agent-server") while normalizing legacy storedkind: "local"values on read.index.htmlwhen they start a same-origin backend.VITE_BACKEND_BASE_URL; remote agent servers are added through the backend dialog./projectsas a workspace parent; only user-stored workspace parents are scanned.Launch Path Screenshots
npm run devnpm run dev:frontendnpm run dev:static -- --skip-buildnode scripts/static-server.mjs --dir buildThe PNGs are committed under
.pr/qa/and listed in.pr/README.md.Verification
npm run typechecknpm testnpm run buildgit diff --checkorigin/main:comm -23 <(git diff --name-only origin/main...HEAD | sort) <(git diff -w --name-only origin/main...HEAD | sort)🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.24.0-pythonopenhands-automation==1.0.0a5d126c4cdb9a001b7f2eedd6f3c6a5a3ee7971132Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-d126c4cRun
All tags pushed for this build
About Multi-Architecture Support
sha-d126c4c) is a multi-arch manifest supporting both amd64 and arm64sha-d126c4c-amd64) are also available if needed