From d18bd6cb4ca5be352a05c896fbc2bc713e0b7a73 Mon Sep 17 00:00:00 2001 From: neubig Date: Fri, 24 Jul 2026 20:00:26 +0000 Subject: [PATCH 1/2] fix: skip redundant ready Cloud onboarding --- AGENTS.md | 2 +- .../onboarding/onboarding-host.test.tsx | 63 ++++++++++++++----- .../features/onboarding/onboarding-host.tsx | 34 +++++++++- 3 files changed, 81 insertions(+), 18 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 342c63556..636e6850b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -584,7 +584,7 @@ When adding code that needs a new string, decide up front which rule it falls un - Changes tab / `FileDiffViewer` deleted-file note: the agent-server's `/api/git/diff` endpoint calls `path.exists()` first (see `openhands-sdk/openhands/sdk/git/git_diff.py` → `get_git_diff`), so requesting a diff for a `D` (deleted) file returns `GitPathError` → HTTP 400 and trips the global QueryCache error toast. `useUnifiedGitDiff` disables the query when `type === "D"` and `FileDiffViewer` renders a localized "file deleted" placeholder (`DIFF_VIEWER$FILE_DELETED`, `data-testid="file-deleted-message"`) instead of the view-mode toolbar / Monaco editor for that case. -- Onboarding modal: `src/components/features/onboarding/onboarding-modal.tsx` is rendered by `` and gated by the `openhands-onboarded` localStorage flag. It tracks logical phases (`backend`, `agent`, `setup`, `hello`) rather than fixed numeric steps; the backend phase may be omitted for an already configured backend. Agent choices are derived from `ACP_PROVIDERS` plus OpenHands. The setup phase renders `SetupLlmStep` for OpenHands or `SetupAcpSecretsStep` for ACP providers. Keep the phase-based navigation so adding or removing the backend slide cannot move users to the wrong step. +- Onboarding modal: `src/components/features/onboarding/onboarding-modal.tsx` is rendered by `` and normally gated by the `openhands-onboarded` localStorage flag. `OnboardingHost` also suppresses the modal, without writing that flag, when the active backend matches the deployment's locked Cloud host and its settings report a usable LLM (non-empty model plus API-key or subscription auth); this readiness exception must not apply to Local or arbitrary Cloud backends. It tracks logical phases (`backend`, `agent`, `setup`, `hello`) rather than fixed numeric steps; the backend phase may be omitted for an already configured backend. Agent choices are derived from `ACP_PROVIDERS` plus OpenHands. The setup phase renders `SetupLlmStep` for OpenHands or `SetupAcpSecretsStep` for ACP providers. Keep the phase-based navigation so adding or removing the backend slide cannot move users to the wrong step. - Files tab diff-view default logic: keyed off `useHasAttachedSource()` (`src/hooks/use-has-attached-source.ts`), which is true when the user explicitly attached _either_ a repo (`conversation.selected_repository`) _or_ a local workspace (`getStoredConversationMetadata(id).selected_workspace`, persisted by `createConversation` when `workingDirOverride` is supplied). The agent-server pre-initialises every conversation workspace as a git worktree for its own change tracking, so do NOT use a filesystem probe (`git status` / `useUnifiedGetGitChanges`) as the attachment signal — that was tried in earlier iterations and made every fresh no-attachment conversation incorrectly default to diff view. The companion `useHasGitCommits` probe (`src/hooks/query/use-has-git-commits.ts`) then suppresses diff view for attached-but-empty cases (unborn HEAD, non-git workspace). diff --git a/__tests__/components/onboarding/onboarding-host.test.tsx b/__tests__/components/onboarding/onboarding-host.test.tsx index 790f89205..507c9f9aa 100644 --- a/__tests__/components/onboarding/onboarding-host.test.tsx +++ b/__tests__/components/onboarding/onboarding-host.test.tsx @@ -1,6 +1,6 @@ import React from "react"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; -import { render, screen } from "@testing-library/react"; +import { render, screen, waitFor } from "@testing-library/react"; import { MemoryRouter } from "react-router"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -108,7 +108,35 @@ describe("OnboardingHost", () => { ).toBeInTheDocument(); }); - it("shows the modal for a fresh Cloud user even when the backend has a configured LLM", async () => { + it("skips the modal when the locked Cloud backend has a configured LLM", async () => { + vi.stubEnv("VITE_LOCK_TO_CLOUD", "https://app.all-hands.dev"); + seedCloudBackend(); + const getSettings = vi + .spyOn(SettingsService, "getSettings") + .mockResolvedValue({ + ...DEFAULT_SETTINGS, + llm_api_key_set: true, + agent_settings: { + ...DEFAULT_SETTINGS.agent_settings, + llm: { model: "anthropic/claude-sonnet-4-5", api_key: "stored" }, + }, + }); + + renderHost(); + + await waitFor(() => { + expect(getSettings).toHaveBeenCalledOnce(); + expect( + screen.queryByTestId("onboarding-modal-stub"), + ).not.toBeInTheDocument(); + }); + expect( + window.localStorage.getItem(ONBOARDING_COMPLETED_STORAGE_KEY), + ).toBeNull(); + }); + + it("still shows the modal when a configured Cloud backend does not match the locked host", async () => { + vi.stubEnv("VITE_LOCK_TO_CLOUD", "https://other-cloud.example.com"); seedCloudBackend(); vi.spyOn(SettingsService, "getSettings").mockResolvedValue({ ...DEFAULT_SETTINGS, @@ -129,28 +157,35 @@ describe("OnboardingHost", () => { ).toBeNull(); }); - it("shows the modal for a fresh Cloud user even when the active LLM uses subscription auth", async () => { + it("skips the modal when the locked Cloud backend uses subscription auth", async () => { + vi.stubEnv("VITE_LOCK_TO_CLOUD", "https://app.all-hands.dev"); seedCloudBackend(); - vi.spyOn(SettingsService, "getSettings").mockResolvedValue({ - ...DEFAULT_SETTINGS, - llm_api_key_set: false, - agent_settings: { - ...DEFAULT_SETTINGS.agent_settings, - llm: { model: "openai/gpt-5.5", auth_type: "subscription" }, - }, - }); + const getSettings = vi + .spyOn(SettingsService, "getSettings") + .mockResolvedValue({ + ...DEFAULT_SETTINGS, + llm_api_key_set: false, + agent_settings: { + ...DEFAULT_SETTINGS.agent_settings, + llm: { model: "openai/gpt-5.5", auth_type: "subscription" }, + }, + }); renderHost(); - expect( - await screen.findByTestId("onboarding-modal-stub"), - ).toBeInTheDocument(); + await waitFor(() => { + expect(getSettings).toHaveBeenCalledOnce(); + expect( + screen.queryByTestId("onboarding-modal-stub"), + ).not.toBeInTheDocument(); + }); expect( window.localStorage.getItem(ONBOARDING_COMPLETED_STORAGE_KEY), ).toBeNull(); }); it("still shows the modal for a Cloud user when an API key is set but no model is configured", async () => { + vi.stubEnv("VITE_LOCK_TO_CLOUD", "https://app.all-hands.dev"); seedCloudBackend(); vi.spyOn(SettingsService, "getSettings").mockResolvedValue({ ...DEFAULT_SETTINGS, diff --git a/src/components/features/onboarding/onboarding-host.tsx b/src/components/features/onboarding/onboarding-host.tsx index d40f7ce42..423b2302d 100644 --- a/src/components/features/onboarding/onboarding-host.tsx +++ b/src/components/features/onboarding/onboarding-host.tsx @@ -1,4 +1,9 @@ import { useLocation } from "react-router"; +import { getLockedCloudHost, isSameCloudHost } from "#/api/agent-server-config"; +import { useActiveBackend } from "#/contexts/active-backend-context"; +import { useSettings } from "#/hooks/query/use-settings"; +import { isSubscriptionLlmConfig } from "#/constants/llm-subscription"; +import type { Settings } from "#/types/settings"; import { OnboardingModal } from "./onboarding-modal"; import { isOnboardingPreviewActive, @@ -6,15 +11,27 @@ import { } from "./onboarding-preview"; import { useOnboardingCompletion } from "./use-onboarding-completion"; +function hasUsableCloudLlm(settings: Settings | undefined): boolean { + const llm = settings?.agent_settings?.llm as + | Record + | undefined; + const hasModel = + typeof llm?.model === "string" && llm.model.trim().length > 0; + const hasAuth = + settings?.llm_api_key_set === true || isSubscriptionLlmConfig(llm); + return hasModel && hasAuth; +} + /** * Mounts the onboarding modal automatically the first time the user * lands on a host route (i.e. when the localStorage onboarding flag * isn't set yet). Closing or completing the flow marks it done so the * modal won't re-appear on subsequent visits. * - * Backend readiness is intentionally not treated as onboarding completion: - * a fresh browser/origin should see onboarding once even when it connects - * to an existing backend that already has an LLM configured. + * A deployment locked to an already-authenticated Cloud backend may provide + * everything onboarding would collect. Once that backend reports a usable LLM, + * suppress the redundant modal without writing a fake completion marker. + * Other Cloud hosts and all Local backends remain browser-onboarding-driven. * * With `?previewOnboardingStep=<0-3>` the modal opens on that slide for * design review without persisting completion (works on any route when @@ -25,9 +42,20 @@ export function OnboardingHost() { const previewStep = readOnboardingPreviewStep(location.search); const isPreview = isOnboardingPreviewActive(location.search); const { isCompleted, markCompleted } = useOnboardingCompletion(); + const { backend } = useActiveBackend(); + const settings = useSettings(); + const lockedCloudHost = getLockedCloudHost(); + const isActiveLockedCloudBackend = + backend.kind === "cloud" && + lockedCloudHost !== null && + isSameCloudHost(backend.host, lockedCloudHost); if (!isPreview) { if (isCompleted) return null; + if (isActiveLockedCloudBackend && settings.isLoading) return null; + if (isActiveLockedCloudBackend && hasUsableCloudLlm(settings.data)) { + return null; + } } const handleClose = () => { From 5ad987d2c989a219a7d8bc1ca0966a8407cc671c Mon Sep 17 00:00:00 2001 From: neubig Date: Sat, 25 Jul 2026 08:16:58 +0000 Subject: [PATCH 2/2] fix: support any ready Cloud backend --- AGENTS.md | 2 +- .../onboarding/onboarding-host.test.tsx | 36 ++++++++++--------- .../features/onboarding/onboarding-host.tsx | 19 ++++------ 3 files changed, 27 insertions(+), 30 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 636e6850b..f32254633 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -584,7 +584,7 @@ When adding code that needs a new string, decide up front which rule it falls un - Changes tab / `FileDiffViewer` deleted-file note: the agent-server's `/api/git/diff` endpoint calls `path.exists()` first (see `openhands-sdk/openhands/sdk/git/git_diff.py` → `get_git_diff`), so requesting a diff for a `D` (deleted) file returns `GitPathError` → HTTP 400 and trips the global QueryCache error toast. `useUnifiedGitDiff` disables the query when `type === "D"` and `FileDiffViewer` renders a localized "file deleted" placeholder (`DIFF_VIEWER$FILE_DELETED`, `data-testid="file-deleted-message"`) instead of the view-mode toolbar / Monaco editor for that case. -- Onboarding modal: `src/components/features/onboarding/onboarding-modal.tsx` is rendered by `` and normally gated by the `openhands-onboarded` localStorage flag. `OnboardingHost` also suppresses the modal, without writing that flag, when the active backend matches the deployment's locked Cloud host and its settings report a usable LLM (non-empty model plus API-key or subscription auth); this readiness exception must not apply to Local or arbitrary Cloud backends. It tracks logical phases (`backend`, `agent`, `setup`, `hello`) rather than fixed numeric steps; the backend phase may be omitted for an already configured backend. Agent choices are derived from `ACP_PROVIDERS` plus OpenHands. The setup phase renders `SetupLlmStep` for OpenHands or `SetupAcpSecretsStep` for ACP providers. Keep the phase-based navigation so adding or removing the backend slide cannot move users to the wrong step. +- Onboarding modal: `src/components/features/onboarding/onboarding-modal.tsx` is rendered by `` and normally gated by the `openhands-onboarded` localStorage flag. `OnboardingHost` also suppresses the modal, without writing that flag, when any active Cloud backend's settings report a usable LLM (non-empty model plus API-key or subscription auth); this readiness exception must not apply to Local backends. It tracks logical phases (`backend`, `agent`, `setup`, `hello`) rather than fixed numeric steps; the backend phase may be omitted for an already configured backend. Agent choices are derived from `ACP_PROVIDERS` plus OpenHands. The setup phase renders `SetupLlmStep` for OpenHands or `SetupAcpSecretsStep` for ACP providers. Keep the phase-based navigation so adding or removing the backend slide cannot move users to the wrong step. - Files tab diff-view default logic: keyed off `useHasAttachedSource()` (`src/hooks/use-has-attached-source.ts`), which is true when the user explicitly attached _either_ a repo (`conversation.selected_repository`) _or_ a local workspace (`getStoredConversationMetadata(id).selected_workspace`, persisted by `createConversation` when `workingDirOverride` is supplied). The agent-server pre-initialises every conversation workspace as a git worktree for its own change tracking, so do NOT use a filesystem probe (`git status` / `useUnifiedGetGitChanges`) as the attachment signal — that was tried in earlier iterations and made every fresh no-attachment conversation incorrectly default to diff view. The companion `useHasGitCommits` probe (`src/hooks/query/use-has-git-commits.ts`) then suppresses diff view for attached-but-empty cases (unborn HEAD, non-git workspace). diff --git a/__tests__/components/onboarding/onboarding-host.test.tsx b/__tests__/components/onboarding/onboarding-host.test.tsx index 507c9f9aa..42860c44b 100644 --- a/__tests__/components/onboarding/onboarding-host.test.tsx +++ b/__tests__/components/onboarding/onboarding-host.test.tsx @@ -108,8 +108,7 @@ describe("OnboardingHost", () => { ).toBeInTheDocument(); }); - it("skips the modal when the locked Cloud backend has a configured LLM", async () => { - vi.stubEnv("VITE_LOCK_TO_CLOUD", "https://app.all-hands.dev"); + it("skips the modal when the active Cloud backend has a configured LLM", async () => { seedCloudBackend(); const getSettings = vi .spyOn(SettingsService, "getSettings") @@ -135,30 +134,34 @@ describe("OnboardingHost", () => { ).toBeNull(); }); - it("still shows the modal when a configured Cloud backend does not match the locked host", async () => { + it("skips the modal for a configured Cloud backend that does not match the locked host", async () => { vi.stubEnv("VITE_LOCK_TO_CLOUD", "https://other-cloud.example.com"); seedCloudBackend(); - vi.spyOn(SettingsService, "getSettings").mockResolvedValue({ - ...DEFAULT_SETTINGS, - llm_api_key_set: true, - agent_settings: { - ...DEFAULT_SETTINGS.agent_settings, - llm: { model: "anthropic/claude-sonnet-4-5", api_key: "stored" }, - }, - }); + const getSettings = vi + .spyOn(SettingsService, "getSettings") + .mockResolvedValue({ + ...DEFAULT_SETTINGS, + llm_api_key_set: true, + agent_settings: { + ...DEFAULT_SETTINGS.agent_settings, + llm: { model: "anthropic/claude-sonnet-4-5", api_key: "stored" }, + }, + }); renderHost(); - expect( - await screen.findByTestId("onboarding-modal-stub"), - ).toBeInTheDocument(); + await waitFor(() => { + expect(getSettings).toHaveBeenCalledOnce(); + expect( + screen.queryByTestId("onboarding-modal-stub"), + ).not.toBeInTheDocument(); + }); expect( window.localStorage.getItem(ONBOARDING_COMPLETED_STORAGE_KEY), ).toBeNull(); }); - it("skips the modal when the locked Cloud backend uses subscription auth", async () => { - vi.stubEnv("VITE_LOCK_TO_CLOUD", "https://app.all-hands.dev"); + it("skips the modal when the active Cloud backend uses subscription auth", async () => { seedCloudBackend(); const getSettings = vi .spyOn(SettingsService, "getSettings") @@ -185,7 +188,6 @@ describe("OnboardingHost", () => { }); it("still shows the modal for a Cloud user when an API key is set but no model is configured", async () => { - vi.stubEnv("VITE_LOCK_TO_CLOUD", "https://app.all-hands.dev"); seedCloudBackend(); vi.spyOn(SettingsService, "getSettings").mockResolvedValue({ ...DEFAULT_SETTINGS, diff --git a/src/components/features/onboarding/onboarding-host.tsx b/src/components/features/onboarding/onboarding-host.tsx index 423b2302d..18d765891 100644 --- a/src/components/features/onboarding/onboarding-host.tsx +++ b/src/components/features/onboarding/onboarding-host.tsx @@ -1,5 +1,4 @@ import { useLocation } from "react-router"; -import { getLockedCloudHost, isSameCloudHost } from "#/api/agent-server-config"; import { useActiveBackend } from "#/contexts/active-backend-context"; import { useSettings } from "#/hooks/query/use-settings"; import { isSubscriptionLlmConfig } from "#/constants/llm-subscription"; @@ -28,10 +27,10 @@ function hasUsableCloudLlm(settings: Settings | undefined): boolean { * isn't set yet). Closing or completing the flow marks it done so the * modal won't re-appear on subsequent visits. * - * A deployment locked to an already-authenticated Cloud backend may provide - * everything onboarding would collect. Once that backend reports a usable LLM, - * suppress the redundant modal without writing a fake completion marker. - * Other Cloud hosts and all Local backends remain browser-onboarding-driven. + * An already-authenticated Cloud backend may provide everything onboarding + * would collect. Once the active Cloud backend reports a usable LLM, suppress + * the redundant modal without writing a fake completion marker. Local backends + * remain browser-onboarding-driven. * * With `?previewOnboardingStep=<0-3>` the modal opens on that slide for * design review without persisting completion (works on any route when @@ -44,16 +43,12 @@ export function OnboardingHost() { const { isCompleted, markCompleted } = useOnboardingCompletion(); const { backend } = useActiveBackend(); const settings = useSettings(); - const lockedCloudHost = getLockedCloudHost(); - const isActiveLockedCloudBackend = - backend.kind === "cloud" && - lockedCloudHost !== null && - isSameCloudHost(backend.host, lockedCloudHost); + const isCloudBackend = backend.kind === "cloud"; if (!isPreview) { if (isCompleted) return null; - if (isActiveLockedCloudBackend && settings.isLoading) return null; - if (isActiveLockedCloudBackend && hasUsableCloudLlm(settings.data)) { + if (isCloudBackend && settings.isLoading) return null; + if (isCloudBackend && hasUsableCloudLlm(settings.data)) { return null; } }