Skip to content

Commit c47e06f

Browse files
camielvsclaude
andcommitted
address pr feedback
- Gate OnboardingWelcome on !isReady with a spinner (match IndexRedirect) - Import APP_ROUTES from appRoutes instead of the router shim Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 4df288c commit c47e06f

12 files changed

Lines changed: 111 additions & 51 deletions

src/components/Onboarding/IndexRedirect.test.tsx

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ vi.mock("@tanstack/react-router", () => ({
77
Navigate: ({ to }: { to: string }) => <div data-testid="navigate">{to}</div>,
88
}));
99

10-
let onboarding = { isReady: true, isComplete: false, dismissed: false };
10+
let onboarding = { isResolved: true, shouldShowOnboarding: true };
1111
vi.mock("@/providers/OnboardingProvider/OnboardingProvider", () => ({
1212
useOnboarding: () => onboarding,
1313
}));
@@ -17,26 +17,20 @@ afterEach(cleanup);
1717
const target = () => screen.queryByTestId("navigate");
1818

1919
describe("IndexRedirect", () => {
20-
it("waits (no redirect) until onboarding state is ready", () => {
21-
onboarding = { isReady: false, isComplete: false, dismissed: false };
20+
it("waits (no redirect) until onboarding state is resolved", () => {
21+
onboarding = { isResolved: false, shouldShowOnboarding: false };
2222
render(<IndexRedirect />);
2323
expect(target()).toBeNull();
2424
});
2525

26-
it("redirects to /welcome while onboarding is active", () => {
27-
onboarding = { isReady: true, isComplete: false, dismissed: false };
26+
it("redirects to /welcome while onboarding should show", () => {
27+
onboarding = { isResolved: true, shouldShowOnboarding: true };
2828
render(<IndexRedirect />);
2929
expect(target()).toHaveTextContent("/welcome");
3030
});
3131

32-
it("redirects to /dashboard once complete", () => {
33-
onboarding = { isReady: true, isComplete: true, dismissed: false };
34-
render(<IndexRedirect />);
35-
expect(target()).toHaveTextContent("/dashboard");
36-
});
37-
38-
it("redirects to /dashboard once dismissed", () => {
39-
onboarding = { isReady: true, isComplete: false, dismissed: true };
32+
it("redirects to /dashboard when onboarding should not show", () => {
33+
onboarding = { isResolved: true, shouldShowOnboarding: false };
4034
render(<IndexRedirect />);
4135
expect(target()).toHaveTextContent("/dashboard");
4236
});

src/components/Onboarding/IndexRedirect.tsx

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,21 +6,20 @@ import { useOnboarding } from "@/providers/OnboardingProvider/OnboardingProvider
66
import { APP_ROUTES } from "@/routes/appRoutes";
77

88
export function IndexRedirect() {
9-
const { isReady, isComplete, dismissed } = useOnboarding();
9+
const { isResolved, shouldShowOnboarding } = useOnboarding();
1010

11-
if (!isReady) {
11+
if (!isResolved) {
1212
return (
1313
<BlockStack align="center" inlineAlign="center" className="h-full">
1414
<Spinner />
1515
</BlockStack>
1616
);
1717
}
1818

19-
const showOnboarding = !isComplete && !dismissed;
2019
return (
2120
<Navigate
2221
replace
23-
to={showOnboarding ? APP_ROUTES.WELCOME : APP_ROUTES.DASHBOARD}
22+
to={shouldShowOnboarding ? APP_ROUTES.WELCOME : APP_ROUTES.DASHBOARD}
2423
/>
2524
);
2625
}

src/components/Onboarding/OnboardingNavPill.test.tsx

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,7 @@ let onboarding = {
1212
steps: [],
1313
completedCount: 1,
1414
total: 4,
15-
isComplete: false,
16-
dismissed: false,
17-
isResolved: true,
15+
shouldShowOnboarding: true,
1816
markDocsRead: vi.fn(),
1917
};
2018
vi.mock("@/providers/OnboardingProvider/OnboardingProvider", () => ({
@@ -26,9 +24,7 @@ function resetState() {
2624
steps: [],
2725
completedCount: 1,
2826
total: 4,
29-
isComplete: false,
30-
dismissed: false,
31-
isResolved: true,
27+
shouldShowOnboarding: true,
3228
markDocsRead: vi.fn(),
3329
};
3430
}
@@ -39,19 +35,13 @@ afterEach(cleanup);
3935
const pill = () => screen.queryByText(/Onboarding/);
4036

4137
describe("OnboardingNavPill", () => {
42-
it("shows progress while onboarding is in progress", () => {
38+
it("shows progress while onboarding should show", () => {
4339
render(<OnboardingNavPill />);
4440
expect(pill()).toHaveTextContent("Onboarding · 1/4");
4541
});
4642

47-
it("is hidden once onboarding is complete", () => {
48-
onboarding.isComplete = true;
49-
render(<OnboardingNavPill />);
50-
expect(pill()).toBeNull();
51-
});
52-
53-
it("is hidden once onboarding is dismissed", () => {
54-
onboarding.dismissed = true;
43+
it("is hidden when onboarding should not show", () => {
44+
onboarding.shouldShowOnboarding = false;
5545
render(<OnboardingNavPill />);
5646
expect(pill()).toBeNull();
5747
});

src/components/Onboarding/OnboardingNavPill.tsx

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,9 @@ import { useOnboarding } from "@/providers/OnboardingProvider/OnboardingProvider
1212
import { tracking } from "@/utils/tracking";
1313

1414
export function OnboardingNavPill() {
15-
const { completedCount, total, isComplete, dismissed, isResolved } =
16-
useOnboarding();
15+
const { completedCount, total, shouldShowOnboarding } = useOnboarding();
1716

18-
if (!isResolved || isComplete || dismissed) {
17+
if (!shouldShowOnboarding) {
1918
return null;
2019
}
2120

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import { cleanup, render, screen } from "@testing-library/react";
2+
import { afterEach, describe, expect, it, vi } from "vitest";
3+
4+
import { OnboardingWelcome } from "./OnboardingWelcome";
5+
6+
vi.mock("@tanstack/react-router", () => ({
7+
Navigate: ({ to }: { to: string }) => <div data-testid="navigate">{to}</div>,
8+
Link: ({ children }: { children: React.ReactNode }) => <a>{children}</a>,
9+
}));
10+
11+
vi.mock("@/components/Learn/OnboardingHero", () => ({
12+
OnboardingHero: () => <div data-testid="hero" />,
13+
}));
14+
15+
let onboarding = { isResolved: true, shouldShowOnboarding: true };
16+
vi.mock("@/providers/OnboardingProvider/OnboardingProvider", () => ({
17+
useOnboarding: () => onboarding,
18+
}));
19+
20+
afterEach(cleanup);
21+
22+
describe("OnboardingWelcome", () => {
23+
it("shows a spinner until onboarding state is resolved", () => {
24+
onboarding = { isResolved: false, shouldShowOnboarding: false };
25+
render(<OnboardingWelcome />);
26+
expect(screen.queryByTestId("hero")).toBeNull();
27+
expect(screen.queryByTestId("navigate")).toBeNull();
28+
});
29+
30+
it("renders the welcome hero when onboarding should show", () => {
31+
onboarding = { isResolved: true, shouldShowOnboarding: true };
32+
render(<OnboardingWelcome />);
33+
expect(screen.getByTestId("hero")).toBeInTheDocument();
34+
});
35+
36+
it("redirects to /dashboard when onboarding should not show", () => {
37+
onboarding = { isResolved: true, shouldShowOnboarding: false };
38+
render(<OnboardingWelcome />);
39+
expect(screen.getByTestId("navigate")).toHaveTextContent("/dashboard");
40+
});
41+
});

src/components/Onboarding/OnboardingWelcome.tsx

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,23 @@ import { Link, Navigate } from "@tanstack/react-router";
22

33
import { OnboardingHero } from "@/components/Learn/OnboardingHero";
44
import { BlockStack } from "@/components/ui/layout";
5+
import { Spinner } from "@/components/ui/spinner";
56
import { useOnboarding } from "@/providers/OnboardingProvider/OnboardingProvider";
6-
import { APP_ROUTES } from "@/routes/router";
7+
import { APP_ROUTES } from "@/routes/appRoutes";
78
import { tracking } from "@/utils/tracking";
89

910
export function OnboardingWelcome() {
10-
const { isReady, isComplete, dismissed } = useOnboarding();
11+
const { isResolved, shouldShowOnboarding } = useOnboarding();
1112

12-
if (isReady && (isComplete || dismissed)) {
13+
if (!isResolved) {
14+
return (
15+
<BlockStack align="center" inlineAlign="center" className="h-full">
16+
<Spinner />
17+
</BlockStack>
18+
);
19+
}
20+
21+
if (!shouldShowOnboarding) {
1322
return <Navigate to={APP_ROUTES.DASHBOARD} replace />;
1423
}
1524

src/providers/BackendProvider.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ export const BackendProvider = ({ children }: { children: ReactNode }) => {
105105
if (!normalizedUrl) {
106106
if (notifyResult) notify("Backend is not configured", "error");
107107
if (saveAvailability) setAvailable(false);
108+
setReady(true);
108109
return Promise.resolve(false);
109110
}
110111
return fetch(`${normalizedUrl}/services/ping`)
@@ -126,6 +127,7 @@ export const BackendProvider = ({ children }: { children: ReactNode }) => {
126127
.catch(() => {
127128
if (notifyResult) notify("Backend unavailable", "error");
128129
if (saveAvailability) setAvailable(false);
130+
setReady(true);
129131
return false;
130132
});
131133
},

src/providers/OnboardingProvider/OnboardingProvider.test.tsx

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,17 @@ describe("OnboardingProvider", () => {
9595
expect(patched()).toBe(false);
9696
});
9797

98+
it("does not show onboarding when the backend is unavailable", async () => {
99+
backend = { available: false, backendUrl: "https://backend.example" };
100+
101+
const { result } = render();
102+
103+
await waitFor(() => expect(result.current.isReady).toBe(true));
104+
expect(result.current.isOnboardingAvailable).toBe(false);
105+
expect(result.current.shouldShowOnboarding).toBe(false);
106+
expect(patched()).toBe(false);
107+
});
108+
98109
it("derives tour and run completion live without persisting them", async () => {
99110
tourCompletions = { "first-pipeline": { completedAt: "x" } };
100111
runsPayload = { pipeline_runs: [{ id: "run-1" }] };

src/providers/OnboardingProvider/OnboardingProvider.tsx

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ interface OnboardingContextValue {
4444
dismissed: boolean;
4545
isReady: boolean;
4646
isResolved: boolean;
47+
isOnboardingAvailable: boolean;
48+
shouldShowOnboarding: boolean;
4749
markDocsRead: () => void;
4850
dismiss: () => void;
4951
reopen: () => void;
@@ -79,7 +81,13 @@ function useHasMyRun(): {
7981
export function OnboardingProvider({ children }: { children: ReactNode }) {
8082
const { track } = useAnalytics();
8183
const notify = useToastNotification();
82-
const { ready: backendReady, configured } = useBackend();
84+
const {
85+
ready: backendReady,
86+
configured,
87+
available,
88+
backendUrl,
89+
} = useBackend();
90+
const hasBackend = available && Boolean(backendUrl);
8391
const { data: progress, isLoading: progressLoading } =
8492
useOnboardingProgress();
8593
const persist = usePersistOnboardingProgress();
@@ -100,8 +108,12 @@ export function OnboardingProvider({ children }: { children: ReactNode }) {
100108
};
101109

102110
const isComplete = ONBOARDING_STEP_IDS.every((id) => desiredSteps[id]);
111+
const dismissed = progress?.dismissed ?? false;
103112
const isReady = !progressLoading && !toursLoading && !runsLoading;
104113
const isResolved = (backendReady || !configured) && isReady;
114+
const isOnboardingAvailable = isResolved && hasBackend;
115+
const shouldShowOnboarding =
116+
isOnboardingAvailable && !isComplete && !dismissed;
105117

106118
const [pipelineWriteCount, setPipelineWriteCount] = useState(0);
107119

@@ -185,9 +197,11 @@ export function OnboardingProvider({ children }: { children: ReactNode }) {
185197
completedCount: steps.filter((step) => step.completed).length,
186198
total: steps.length,
187199
isComplete,
188-
dismissed: progress?.dismissed ?? false,
200+
dismissed,
189201
isReady,
190202
isResolved,
203+
isOnboardingAvailable,
204+
shouldShowOnboarding,
191205
markDocsRead,
192206
dismiss,
193207
reopen,

src/routes/Dashboard/DashboardLayout.tsx

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,7 @@ export function DashboardLayout() {
5959
const requiresAuthorization = isAuthorizationRequired();
6060
const isComponentSearchEnabled = useFlagValue("component-search-v2");
6161

62-
const { isComplete, dismissed, isResolved } = useOnboarding();
63-
const showOnboarding = isResolved && !isComplete && !dismissed;
62+
const { shouldShowOnboarding } = useOnboarding();
6463

6564
const baseItems = isComponentSearchEnabled
6665
? BASE_SIDEBAR_ITEMS.map((item) =>
@@ -70,11 +69,11 @@ export function DashboardLayout() {
7069
)
7170
: BASE_SIDEBAR_ITEMS;
7271

73-
const sidebarItems: SidebarItem[] = showOnboarding
72+
const sidebarItems: SidebarItem[] = shouldShowOnboarding
7473
? [
7574
{
7675
to: APP_ROUTES.WELCOME,
77-
label: "Get started",
76+
label: "Get Started",
7877
icon: "Rocket",
7978
exact: true,
8079
},

0 commit comments

Comments
 (0)