From a44a7255079a6c419e11cda8d174869691a67ceb Mon Sep 17 00:00:00 2001 From: Alejandro Bailo <59607668+alejandrobailo@users.noreply.github.com> Date: Wed, 30 Sep 2026 12:34:52 +0200 Subject: [PATCH] fix(ui): retry the first-run redirect until the add-provider wizard opens (#12914) --- ...t-run-redirect-until-wizard-opens.fixed.md | 1 + .../__tests__/onboarding-gate.test.tsx | 120 +++++++++++++++--- ui/components/onboarding/onboarding-gate.tsx | 33 ++++- ui/lib/onboarding/first-run-marker.ts | 46 ++++++- 4 files changed, 169 insertions(+), 31 deletions(-) create mode 100644 ui/changelog.d/ui-first-run-redirect-until-wizard-opens.fixed.md diff --git a/ui/changelog.d/ui-first-run-redirect-until-wizard-opens.fixed.md b/ui/changelog.d/ui-first-run-redirect-until-wizard-opens.fixed.md new file mode 100644 index 0000000000..e2d6c268ee --- /dev/null +++ b/ui/changelog.d/ui-first-run-redirect-until-wizard-opens.fixed.md @@ -0,0 +1 @@ +First-login redirect to the add-provider wizard is retried on the next load when the navigation was cut short, on Cloud and self-hosted alike, instead of being written off after a single attempt diff --git a/ui/components/onboarding/__tests__/onboarding-gate.test.tsx b/ui/components/onboarding/__tests__/onboarding-gate.test.tsx index f83c8a0385..a6796dd005 100644 --- a/ui/components/onboarding/__tests__/onboarding-gate.test.tsx +++ b/ui/components/onboarding/__tests__/onboarding-gate.test.tsx @@ -1,7 +1,15 @@ -import { render, waitFor } from "@testing-library/react"; +import { act, render, waitFor } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { isFirstRunHandled } from "@/lib/onboarding/first-run-marker"; +import { + FIRST_RUN_MAX_ATTEMPTS, + isFirstRunHandled, +} from "@/lib/onboarding/first-run-marker"; +import { + dispatchProviderFunnel, + PROVIDER_FUNNEL_STEP, + WIZARD_OPEN_SOURCE, +} from "@/lib/provider-funnel/provider-funnel-events"; import { addProviderTour } from "@/lib/tours/add-provider.tour"; import { localStorageAdapter } from "@/lib/tours/store/local-storage-adapter"; @@ -111,7 +119,7 @@ describe("OnboardingGate", () => { expect(armMock).toHaveBeenCalledOnce(); }); - it("happens only once per tenant on this browser", async () => { + it("tries again on the next load when the wizard never opened (the navigation was cut short)", async () => { // Given const { unmount } = render( , @@ -124,28 +132,83 @@ describe("OnboardingGate", () => { render(); // Then - expect(isFirstRunHandled(TENANT_A)).toBe(true); - expect(replaceMock).not.toHaveBeenCalled(); + await waitFor(() => + expect(replaceMock).toHaveBeenCalledExactlyOnceWith( + CLOUD_FIRST_RUN_HREF, + ), + ); + expect(isFirstRunHandled(TENANT_A)).toBe(false); }); - it("honours a browser-wide marker written before markers were tenant-scoped", () => { - // Given: e2e storage state and pre-existing browsers set the bare key. - window.localStorage.setItem("prowler.onboarding.first-run", "true"); - - // When - render(); - - // Then - expect(replaceMock).not.toHaveBeenCalled(); - expect(isFirstRunHandled(TENANT_A)).toBe(true); - }); - - it("still runs for a different empty tenant on the same browser", async () => { + it("is resolved once the add-provider wizard opens, so later loads leave the user alone", async () => { // Given const { unmount } = render( , ); await waitFor(() => expect(replaceMock).toHaveBeenCalledOnce()); + act(() => { + dispatchProviderFunnel({ + step: PROVIDER_FUNNEL_STEP.WIZARD_OPENED, + source: WIZARD_OPEN_SOURCE.FIRST_RUN, + }); + }); + unmount(); + replaceMock.mockClear(); + + // When + render(); + + // Then + expect(isFirstRunHandled(TENANT_A)).toBe(true); + expect(replaceMock).not.toHaveBeenCalled(); + }); + + it("gives up after a few attempts that never reached the wizard, so no browser is trapped", async () => { + // Given: three loads whose navigation never completed. + for (let attempt = 0; attempt < FIRST_RUN_MAX_ATTEMPTS; attempt++) { + const { unmount } = render( + , + ); + await waitFor(() => expect(replaceMock).toHaveBeenCalledOnce()); + unmount(); + replaceMock.mockClear(); + } + + // When + render(); + + // Then + expect(isFirstRunHandled(TENANT_A)).toBe(true); + expect(replaceMock).not.toHaveBeenCalled(); + }); + + it.each(["true", "1legacy", "-1"])( + "honours a browser-wide marker holding %s, written before markers counted attempts", + (value) => { + // Given: e2e storage state and pre-existing browsers set the bare key. + window.localStorage.setItem("prowler.onboarding.first-run", value); + + // When + render(); + + // Then + expect(replaceMock).not.toHaveBeenCalled(); + expect(isFirstRunHandled(TENANT_A)).toBe(true); + }, + ); + + it("still runs for a different empty tenant on the same browser", async () => { + // Given: tenant A went through its first run on this browser. + const { unmount } = render( + , + ); + await waitFor(() => expect(replaceMock).toHaveBeenCalledOnce()); + act(() => { + dispatchProviderFunnel({ + step: PROVIDER_FUNNEL_STEP.WIZARD_OPENED, + source: WIZARD_OPEN_SOURCE.FIRST_RUN, + }); + }); unmount(); replaceMock.mockClear(); @@ -154,7 +217,8 @@ describe("OnboardingGate", () => { // Then await waitFor(() => expect(replaceMock).toHaveBeenCalledOnce()); - expect(isFirstRunHandled(TENANT_B)).toBe(true); + expect(isFirstRunHandled(TENANT_A)).toBe(true); + expect(isFirstRunHandled(TENANT_B)).toBe(false); }); }); @@ -172,6 +236,24 @@ describe("OnboardingGate", () => { ); expect(armMock).not.toHaveBeenCalled(); }); + + it("tries again on the next load when the wizard never opened, with no tenant id available", async () => { + // Given: self-hosted layouts mount the gate without a tenant id. + vi.stubEnv("UI_CLOUD_ENABLED", "false"); + const { unmount } = render(); + await waitFor(() => expect(replaceMock).toHaveBeenCalledOnce()); + unmount(); + replaceMock.mockClear(); + + // When + render(); + + // Then + await waitFor(() => + expect(replaceMock).toHaveBeenCalledExactlyOnceWith(OSS_FIRST_RUN_HREF), + ); + expect(isFirstRunHandled()).toBe(false); + }); }); describe("when the user cannot add providers", () => { diff --git a/ui/components/onboarding/onboarding-gate.tsx b/ui/components/onboarding/onboarding-gate.tsx index 6f76318415..f2a56cdaac 100644 --- a/ui/components/onboarding/onboarding-gate.tsx +++ b/ui/components/onboarding/onboarding-gate.tsx @@ -12,8 +12,14 @@ import { import { isFirstRunHandled, markFirstRunHandled, + recordFirstRunAttempt, } from "@/lib/onboarding/first-run-marker"; -import { WIZARD_OPEN_SOURCE } from "@/lib/provider-funnel/provider-funnel-events"; +import { + PROVIDER_FUNNEL_EVENT, + PROVIDER_FUNNEL_STEP, + type ProviderFunnelDetail, + WIZARD_OPEN_SOURCE, +} from "@/lib/provider-funnel/provider-funnel-events"; import { buildAddProviderHref } from "@/lib/providers-navigation"; import { isCloud } from "@/lib/shared/env"; import { localStorageAdapter } from "@/lib/tours/store/local-storage-adapter"; @@ -28,7 +34,8 @@ interface OnboardingGateProps { } // New-tenant gate. Mounted once in the layout: an empty tenant is sent straight to -// the add-provider wizard, once per tenant and browser. Renders nothing. +// the add-provider wizard, retried per load until the wizard opens once for that +// tenant on this browser (bounded attempts). Renders nothing. export function OnboardingGate({ hasProviders, tenantId = null, @@ -77,17 +84,29 @@ function FirstRunRedirect({ flow, tenantId }: FirstRunRedirectProps) { return; } - markFirstRunHandled(tenantId); + // The wizard opening resolves the first run, whether this redirect got there + // or the user opened it on their own. Until then each load retries, bounded + // by the attempt count, so a navigation cut short is not the end of it. + const resolveOnWizardOpened = (event: Event) => { + const { detail } = event as CustomEvent; + if (detail?.step === PROVIDER_FUNNEL_STEP.WIZARD_OPENED) { + markFirstRunHandled(tenantId); + } + }; + window.addEventListener(PROVIDER_FUNNEL_EVENT, resolveOnWizardOpened); + recordFirstRunAttempt(tenantId); const addProviderHref = buildAddProviderHref(WIZARD_OPEN_SOURCE.FIRST_RUN); if (!isCloud()) { router.replace(addProviderHref); - return; + } else { + // Tours and the post-connect checkpoint are Cloud-only. + useOnboardingCheckpointStore.getState().arm(); + router.replace(`${addProviderHref}&onboarding=${flow.id}`); } - // Tours and the post-connect checkpoint are Cloud-only. - useOnboardingCheckpointStore.getState().arm(); - router.replace(`${addProviderHref}&onboarding=${flow.id}`); + return () => + window.removeEventListener(PROVIDER_FUNNEL_EVENT, resolveOnWizardOpened); }); return null; diff --git a/ui/lib/onboarding/first-run-marker.ts b/ui/lib/onboarding/first-run-marker.ts index 12e4a52ec9..0b57262602 100644 --- a/ui/lib/onboarding/first-run-marker.ts +++ b/ui/lib/onboarding/first-run-marker.ts @@ -3,11 +3,20 @@ // written there; without this marker an empty tenant would be redirected on // every page load. // +// The first run is resolved when the add-provider wizard actually opens, not +// when the redirect is issued: a navigation cut short (a second login, a tab +// closed mid-flight) must be retried on the next load. Each redirect counts as +// an attempt; after a few attempts that never reached the wizard the marker +// resolves anyway, so a browser can never be trapped in the redirect. +// // Scoped per tenant, like the other onboarding markers: going through the first // run in one tenant must not silence it for another one on the same browser. // The bare key is a browser-wide opt-out: written before markers were scoped, // by e2e storage state, or when no usable tenant id exists. const FIRST_RUN_MARKER_KEY = "prowler.onboarding.first-run"; +const HANDLED_VALUE = "true"; + +export const FIRST_RUN_MAX_ATTEMPTS = 3; // Tenant ids are UUIDs; anything else is refused rather than concatenated // into a storage key. @@ -21,23 +30,50 @@ export function firstRunMarkerKey(tenantId?: string | null): string { return `${FIRST_RUN_MARKER_KEY}.${tenantId.toLowerCase()}`; } +// A stored value is either an attempt count (digits only) or `HANDLED_VALUE`; +// anything else (a legacy or hand-written marker) is read as resolved. +const ATTEMPT_COUNT_PATTERN = /^\d+$/; + +function readAttempts(value: string | null): number | null { + if (value === null) return 0; + return ATTEMPT_COUNT_PATTERN.test(value) ? Number(value) : null; +} + export function isFirstRunHandled(tenantId?: string | null): boolean { if (typeof window === "undefined") return true; try { - return ( - window.localStorage.getItem(FIRST_RUN_MARKER_KEY) !== null || - window.localStorage.getItem(firstRunMarkerKey(tenantId)) !== null + // The bare key opts the whole browser out, unless it merely holds the + // attempt count of a deployment that mounts the gate without a tenant id. + const bareAttempts = readAttempts( + window.localStorage.getItem(FIRST_RUN_MARKER_KEY), ); + if (bareAttempts === null) return true; + const attempts = readAttempts( + window.localStorage.getItem(firstRunMarkerKey(tenantId)), + ); + return attempts === null || attempts >= FIRST_RUN_MAX_ATTEMPTS; } catch { // Unreadable storage must not redirect forever: treat as handled. return true; } } -export function markFirstRunHandled(tenantId?: string | null): void { +export function recordFirstRunAttempt(tenantId?: string | null): void { if (typeof window === "undefined") return; try { - window.localStorage.setItem(firstRunMarkerKey(tenantId), "true"); + const key = firstRunMarkerKey(tenantId); + const attempts = readAttempts(window.localStorage.getItem(key)); + if (attempts === null) return; + window.localStorage.setItem(key, String(attempts + 1)); + } catch { + // Non-fatal: a repeated redirect beats a thrown render. + } +} + +export function markFirstRunHandled(tenantId?: string | null): void { + if (typeof window === "undefined") return; + try { + window.localStorage.setItem(firstRunMarkerKey(tenantId), HANDLED_VALUE); } catch { // Non-fatal: a repeated redirect beats a thrown render. }