fix(ui): retry the first-run redirect until the add-provider wizard opens (#12916)

Co-authored-by: Alejandro Bailo <59607668+alejandrobailo@users.noreply.github.com>

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

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(
         <OnboardingGate hasProviders={false} tenantId={TENANT_A} />,
@@ -124,28 +132,83 @@ describe("OnboardingGate", () => {
       render(<OnboardingGate hasProviders={false} tenantId={TENANT_A} />);
 
       // 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(<OnboardingGate hasProviders={false} tenantId={TENANT_A} />);
-
-      // 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(
         <OnboardingGate hasProviders={false} tenantId={TENANT_A} />,
       );
       await waitFor(() => expect(replaceMock).toHaveBeenCalledOnce());
+      act(() => {
+        dispatchProviderFunnel({
+          step: PROVIDER_FUNNEL_STEP.WIZARD_OPENED,
+          source: WIZARD_OPEN_SOURCE.FIRST_RUN,
+        });
+      });
+      unmount();
+      replaceMock.mockClear();
+
+      // When
+      render(<OnboardingGate hasProviders={false} tenantId={TENANT_A} />);
+
+      // 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(
+          <OnboardingGate hasProviders={false} tenantId={TENANT_A} />,
+        );
+        await waitFor(() => expect(replaceMock).toHaveBeenCalledOnce());
+        unmount();
+        replaceMock.mockClear();
+      }
+
+      // When
+      render(<OnboardingGate hasProviders={false} tenantId={TENANT_A} />);
+
+      // 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(<OnboardingGate hasProviders={false} tenantId={TENANT_A} />);
+
+        // 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(
+        <OnboardingGate hasProviders={false} tenantId={TENANT_A} />,
+      );
+      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(<OnboardingGate hasProviders={false} />);
+      await waitFor(() => expect(replaceMock).toHaveBeenCalledOnce());
+      unmount();
+      replaceMock.mockClear();
+
+      // When
+      render(<OnboardingGate hasProviders={false} />);
+
+      // Then
+      await waitFor(() =>
+        expect(replaceMock).toHaveBeenCalledExactlyOnceWith(OSS_FIRST_RUN_HREF),
+      );
+      expect(isFirstRunHandled()).toBe(false);
+    });
   });
 
   describe("when the user cannot add providers", () => {

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<ProviderFunnelDetail>;
+      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;

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.
   }