diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index cd5dde3032..1ea23016cf 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState, useMemo } from "react"; +import { useEffect, useState, useMemo, useRef } from "react"; import { useQuery, useQueryClient } from "@tanstack/react-query"; import type { AdapterEnvironmentTestResult } from "@paperclipai/shared"; import { useLocation, useNavigate, useParams } from "@/lib/router"; @@ -43,7 +43,10 @@ import { DEFAULT_CODEX_LOCAL_BYPASS_APPROVALS_AND_SANDBOX } from "@paperclipai/a import { DEFAULT_CURSOR_LOCAL_MODEL } from "@paperclipai/adapter-cursor-local"; import { DEFAULT_GEMINI_LOCAL_MODEL } from "@paperclipai/adapter-gemini-local"; import { DEFAULT_OPENCODE_LOCAL_MODEL, isValidOpenCodeModelId } from "@paperclipai/adapter-opencode-local"; -import { resolveRouteOnboardingOptions } from "../lib/onboarding-route"; +import { + companyPrefixFromOnboardingPath, + resolveRouteOnboardingOptions, +} from "../lib/onboarding-route"; import { AsciiArtAnimation } from "./AsciiArtAnimation"; import { FrontDoor } from "./FrontDoor"; import { AgentCapsule } from "./AgentCapsule"; @@ -134,7 +137,13 @@ export function OnboardingWizard() { const queryClient = useQueryClient(); const navigate = useNavigate(); const location = useLocation(); - const { companyPrefix } = useParams<{ companyPrefix?: string }>(); + const { companyPrefix: matchedCompanyPrefix } = useParams<{ companyPrefix?: string }>(); + // This component renders beside ``, not inside it (`App.tsx`), so it + // has no route match and `useParams()` gives nothing. Read the prefix from + // the pathname, which `useLocation()` supplies without a match. The param is + // kept first so a future move inside the route tree needs no change here. + const companyPrefix = + matchedCompanyPrefix ?? companyPrefixFromOnboardingPath(location.pathname); // Support opening the wizard from a route (e.g. /onboarding or an existing // company's "add agent" entry point) in addition to the dialog context. @@ -221,6 +230,16 @@ export function OnboardingWizard() { (saved?.createdIssueRef as string) ?? null ); + // The company the *route* last supplied, so a navigation that stops naming + // one can drop it without touching a company the wizard created itself. + const routeCompanyIdRef = useRef(null); + // The current company, mirrored so the sync effect can read it without + // taking it as a dependency. Depending on it would re-run the effect on + // every company change, and the effect also calls setStep - it would drag + // the user back to the route's initial step mid-flow. + const createdCompanyIdRef = useRef(null); + createdCompanyIdRef.current = createdCompanyId; + // Reset the route-dismissed flag when navigating to a different path. useEffect(() => { setRouteDismissed(false); @@ -234,9 +253,33 @@ export function OnboardingWizard() { if (effectiveOnboardingOptions.initialStep) { setStep(effectiveOnboardingOptions.initialStep); } - if (effectiveOnboardingOptions.companyId) { - setCreatedCompanyId(effectiveOnboardingOptions.companyId); + const routeCompanyId = effectiveOnboardingOptions.companyId ?? null; + if (routeCompanyId) { + // Claim ownership only when the route *introduces* a company. A route + // that merely names the one already in hand - the wizard created it, + // then the user navigated to that company's onboarding path - has not + // supplied anything, so it must not take ownership of it. Otherwise + // navigating on to `/onboarding` would clear work the wizard did. + if (routeCompanyId !== createdCompanyIdRef.current) { + setCreatedCompanyId(routeCompanyId); + setCreatedCompanyPrefix(null); + routeCompanyIdRef.current = routeCompanyId; + } + return; + } + if (routeCompanyIdRef.current) { + // The route named a company and now does not - the user navigated from + // an existing company's onboarding to `/onboarding`, or to a prefix that + // matches nothing. Drop it. Keeping it leaves the wizard showing step 1, + // "create a company", while still holding the previous one, so the next + // confirmation writes into that company instead of making a new one. + // + // Only a company this route supplied is cleared. One the wizard created + // itself, or restored from saved state, is left alone: the ref is null + // in those cases, and clearing them would discard real progress. + setCreatedCompanyId(null); setCreatedCompanyPrefix(null); + routeCompanyIdRef.current = null; } }, [ effectiveOnboardingOpen, diff --git a/ui/src/lib/onboarding-route.test.ts b/ui/src/lib/onboarding-route.test.ts index 30405f9172..ff3156f7a8 100644 --- a/ui/src/lib/onboarding-route.test.ts +++ b/ui/src/lib/onboarding-route.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "vitest"; import { + companyPrefixFromOnboardingPath, isOnboardingPath, isOnboardingWizardActive, resolveRouteOnboardingOptions, @@ -99,3 +100,90 @@ describe("isOnboardingWizardActive", () => { ).toBe(true); }); }); + +describe("companyPrefixFromOnboardingPath", () => { + it("reads the prefix from a company onboarding path", () => { + expect(companyPrefixFromOnboardingPath("/PC7409/onboarding")).toBe("PC7409"); + }); + + it("keeps the prefix as written so the caller decides how to compare it", () => { + // resolveRouteOnboardingOptions already matches case-insensitively. + // Normalising here as well would hide which half owns the comparison. + expect(companyPrefixFromOnboardingPath("/pc7409/Onboarding")).toBe("pc7409"); + }); + + it("has no prefix to read on the unprefixed route", () => { + expect(companyPrefixFromOnboardingPath("/onboarding")).toBeUndefined(); + }); + + it("ignores paths that only look like onboarding", () => { + expect(companyPrefixFromOnboardingPath("/PC7409/onboarding/extra")).toBeUndefined(); + expect(companyPrefixFromOnboardingPath("/PC7409/dashboard")).toBeUndefined(); + expect(companyPrefixFromOnboardingPath("/")).toBeUndefined(); + }); + + it("agrees with isOnboardingPath about what an onboarding path is", () => { + // The two parse the same shape. If they ever disagree the wizard would + // open on a path that resolves no company, or resolve a company on a path + // that is not onboarding. + for (const pathname of ["/onboarding", "/PC1/onboarding", "/PC1/dash", "/a/b/c"]) { + const prefix = companyPrefixFromOnboardingPath(pathname); + if (prefix !== undefined) expect(isOnboardingPath(pathname)).toBe(true); + } + }); + + it("feeds resolveRouteOnboardingOptions the prefix useParams cannot supply", () => { + // The regression this fixes: the wizard renders beside , so + // useParams() returned nothing and every company route opened at step 1. + const companies = [{ id: "c1", issuePrefix: "PC7409" }]; + const pathname = "/PC7409/onboarding"; + + expect( + resolveRouteOnboardingOptions({ pathname, companyPrefix: undefined, companies }), + ).toEqual({ initialStep: 1 }); + + expect( + resolveRouteOnboardingOptions({ + pathname, + companyPrefix: companyPrefixFromOnboardingPath(pathname), + companies, + }), + ).toEqual({ initialStep: 2, companyId: "c1" }); + }); +}); + +describe("navigating away from a company's onboarding route", () => { + const companies = [{ id: "c1", issuePrefix: "PC1" }]; + + // The wizard is a persistent overlay, so it survives navigation and keeps + // its state. Once the route can supply a companyId - which it could not + // before companyPrefixFromOnboardingPath existed - leaving that route has to + // withdraw it, or the wizard shows "create a company" while still holding + // the previous one. + it("stops supplying a company once the path no longer names one", () => { + const onCompanyRoute = resolveRouteOnboardingOptions({ + pathname: "/PC1/onboarding", + companyPrefix: companyPrefixFromOnboardingPath("/PC1/onboarding"), + companies, + }); + expect(onCompanyRoute).toEqual({ initialStep: 2, companyId: "c1" }); + + const afterNavigating = resolveRouteOnboardingOptions({ + pathname: "/onboarding", + companyPrefix: companyPrefixFromOnboardingPath("/onboarding"), + companies, + }); + expect(afterNavigating).toEqual({ initialStep: 1 }); + expect(afterNavigating?.companyId).toBeUndefined(); + }); + + it("supplies no company for a prefix that matches nothing", () => { + expect( + resolveRouteOnboardingOptions({ + pathname: "/NOPE/onboarding", + companyPrefix: companyPrefixFromOnboardingPath("/NOPE/onboarding"), + companies, + }), + ).toEqual({ initialStep: 1 }); + }); +}); diff --git a/ui/src/lib/onboarding-route.ts b/ui/src/lib/onboarding-route.ts index 1d26224b6d..c616a6286e 100644 --- a/ui/src/lib/onboarding-route.ts +++ b/ui/src/lib/onboarding-route.ts @@ -17,6 +17,27 @@ export function isOnboardingPath(pathname: string): boolean { return false; } +/** + * The company prefix in an onboarding pathname, or undefined when there is + * none. + * + * `OnboardingWizard` renders as a full-screen overlay beside `` rather + * than inside it (`App.tsx`), so it has no route match and `useParams()` + * returns nothing there — `companyPrefix` was always undefined and the wizard + * always opened at step 1, even when the URL named a company. `useLocation()` + * needs only the router, not a match, so the pathname is the signal that + * survives where params do not. + * + * Deliberately parses the same shape as {@link isOnboardingPath}: the prefix is + * the first of exactly two segments. Anything else has no prefix to read. + */ +export function companyPrefixFromOnboardingPath(pathname: string): string | undefined { + const segments = pathname.split("/").filter(Boolean); + if (segments.length !== 2) return undefined; + if (segments[1]?.toLowerCase() !== "onboarding") return undefined; + return segments[0]; +} + export function resolveRouteOnboardingOptions(params: { pathname: string; companyPrefix?: string;