Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions components/Onboarding/AddArtistForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { useState } from "react";
import { Plus } from "lucide-react";
import { Button } from "@/components/ui/button";
import SpotifyArtistSearch from "@/components/Artists/SpotifyArtistSearch";
import { useAddRosterArtist } from "@/hooks/onboarding/useAddRosterArtist";
import { useAddSpotifyArtist } from "@/hooks/useAddSpotifyArtist";
import type { SpotifyArtistSearchResult } from "@/types/spotify";

/**
Expand All @@ -15,16 +15,19 @@ import type { SpotifyArtistSearchResult } from "@/types/spotify";
* structurally unreachable for anyone who arrived without a funnel valuation
* (chat#1889). Picking a real Spotify artist resolves the id, profile URL, and
* avatar in one step, reusing the same typeahead as verify-socials (chat#1882).
*
* Adds through `useAddSpotifyArtist` — the same hook the `/artists` dialog uses
* — because it also kicks `POST /api/valuation` for the picked artist when the
* account has no catalog yet. Without that seeding an account onboarded here
* finished setup with nothing to value, so the reward on the completion panel
* could only ever render its "Claim your catalog" fallback (chat#1889 row 8).
*/
const AddArtistForm = () => {
const { addArtist, isAdding } = useAddRosterArtist();
const { add, isAdding } = useAddSpotifyArtist();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Wait for catalogs before relying on seeding

When /setup/artists is opened directly and the user selects an artist before useCatalogs() has resolved, this swapped-in hook reports success but never starts runValuation: useAddSpotifyArtist only seeds when catalogsQuery.data?.catalogs is already a defined empty array, while ConfirmRosterStep renders this form without waiting for that query. That leaves cold-start setup users in the same no-catalog fallback this change is meant to eliminate, so the add flow should wait/refetch until catalog state is known before deciding whether to seed.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Switching AddArtistForm to useAddSpotifyArtist means catalog seeding now depends on catalogsQuery having already resolved (catalogsQuery.data?.catalogs being a defined empty array). If ConfirmRosterStep renders this form before that query settles (e.g. when /setup/artists is opened directly), the add will report success but skip kicking off runValuation, leaving the account without a catalog — the exact dead-end this PR is trying to fix. Consider waiting for/refetching catalog state before deciding whether to seed.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Onboarding/AddArtistForm.tsx, line 26:

<comment>Switching AddArtistForm to useAddSpotifyArtist means catalog seeding now depends on catalogsQuery having already resolved (`catalogsQuery.data?.catalogs` being a defined empty array). If ConfirmRosterStep renders this form before that query settles (e.g. when /setup/artists is opened directly), the add will report success but skip kicking off `runValuation`, leaving the account without a catalog — the exact dead-end this PR is trying to fix. Consider waiting for/refetching catalog state before deciding whether to seed.</comment>

<file context>
@@ -15,16 +15,19 @@ import type { SpotifyArtistSearchResult } from "@/types/spotify";
  */
 const AddArtistForm = () => {
-  const { addArtist, isAdding } = useAddRosterArtist();
+  const { add, isAdding } = useAddSpotifyArtist();
   const [isOpen, setIsOpen] = useState(false);
 
</file context>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve selection when adding setup artists

When a valuation-funnel account already has a catalog and uses this setup form to add another artist, this swapped-in hook also selects and persists the newly created artist via getArtists(created.account_id). The setup payoff renders through useHomeValuation, which scopes measurements to the selected artist and hides whole-catalog results when the echoed scope differs; because no new valuation is kicked when a catalog already exists, the newly added artist has no measured catalog and the user loses the existing baseline valuation in the done panel or /setup/valuation. Use an onboarding path that refreshes without selecting, or restore the previous selection after the add.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Switching AddArtistForm to useAddSpotifyArtist means adding a second artist during setup will select and persist that new artist via getArtists, but useAddSpotifyArtist only triggers POST /api/valuation for the first artist when no catalog exists yet. If an account already has a catalog and baseline valuation, adding another artist here selects it without any valuation, and since useHomeValuation scopes results to the selected artist, the previously-computed baseline valuation would disappear from the done panel / /setup/valuation. Consider refreshing without changing the selection, or restoring the prior selection after the add when a catalog already exists.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Onboarding/AddArtistForm.tsx, line 26:

<comment>Switching AddArtistForm to useAddSpotifyArtist means adding a second artist during setup will select and persist that new artist via getArtists, but useAddSpotifyArtist only triggers POST /api/valuation for the first artist when no catalog exists yet. If an account already has a catalog and baseline valuation, adding another artist here selects it without any valuation, and since useHomeValuation scopes results to the selected artist, the previously-computed baseline valuation would disappear from the done panel / /setup/valuation. Consider refreshing without changing the selection, or restoring the prior selection after the add when a catalog already exists.</comment>

<file context>
@@ -15,16 +15,19 @@ import type { SpotifyArtistSearchResult } from "@/types/spotify";
  */
 const AddArtistForm = () => {
-  const { addArtist, isAdding } = useAddRosterArtist();
+  const { add, isAdding } = useAddSpotifyArtist();
   const [isOpen, setIsOpen] = useState(false);
 
</file context>

const [isOpen, setIsOpen] = useState(false);

const handleSelect = async (artist: SpotifyArtistSearchResult) => {
const added = await addArtist(artist.name, {
profileUrl: artist.external_urls.spotify,
image: artist.images?.[0]?.url,
});
const added = await add(artist);
if (added) setIsOpen(false);
};

Expand Down
31 changes: 31 additions & 0 deletions components/Onboarding/MeasuringCatalogPanel.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
"use client";

import Link from "next/link";
import { buttonVariants } from "@/components/ui/button";
import { cn } from "@/lib/utils";

/**
* Terminal state for `/setup/valuation` while a catalog exists but has no
* valuation yet.
*
* Seeding (chat#1889 row 8) creates the catalog seconds after the first artist
* is added and its measurements land later, so this window is routine. Without
* a state of its own the route could neither redirect (there IS a catalog) nor
* render the hero, and sat on a skeleton with no exit.
*/
const MeasuringCatalogPanel = () => (
<section className="mx-auto flex w-full max-w-xl flex-col items-center gap-4 px-6 py-8 text-center">
<h1 className="text-2xl font-semibold text-foreground">
Measuring your catalog
</h1>
<p className="text-sm text-muted-foreground">
We are pulling play counts for every track. This usually takes a minute.
Your baseline value appears here as soon as it is ready.
</p>
<Link href="/setup/tasks" className={cn(buttonVariants(), "min-w-[200px]")}>
Set up your weekly report
</Link>
</section>
);

export default MeasuringCatalogPanel;
7 changes: 6 additions & 1 deletion components/Onboarding/SetupValuation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { useRouter } from "next/navigation";
import { buttonVariants } from "@/components/ui/button";
import { Skeleton } from "@/components/ui/skeleton";
import ValuationHero from "@/components/Home/ValuationHero";
import MeasuringCatalogPanel from "@/components/Onboarding/MeasuringCatalogPanel";
import useCatalogs from "@/hooks/useCatalogs";
import useHomeValuation from "@/hooks/useHomeValuation";
import { cn } from "@/lib/utils";
Expand Down Expand Up @@ -36,7 +37,7 @@ const SetupValuation = () => {
if (!hasCatalog || isError) router.replace("/catalogs");
}, [isPending, hasCatalog, isError, router]);

if (isPending || !valuation.show) {
if (isPending) {
return (
<div className="mx-auto flex w-full max-w-xl flex-col gap-4 px-6 py-8">
<Skeleton className="h-8 w-1/2 rounded-lg" />
Expand All @@ -45,6 +46,10 @@ const SetupValuation = () => {
);
}

// A catalog exists but has no valuation yet: routine while a seeded
// valuation is still measuring (chat#1889 row 8).
if (!valuation.show) return <MeasuringCatalogPanel />;

return (
<section className="mx-auto flex w-full max-w-xl flex-col items-center gap-4 px-6 py-8">
<ValuationHero
Expand Down
26 changes: 15 additions & 11 deletions components/Onboarding/__tests__/AddArtistForm.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,20 @@ import { fireEvent, render, screen } from "@testing-library/react";
import { beforeEach, describe, expect, it, vi } from "vitest";
import AddArtistForm from "@/components/Onboarding/AddArtistForm";

const addArtist = vi.fn();
const add = vi.fn();

const SPOTIFY_RESULT = {
id: "3TVXtAsR1Inumwj472S9r4",
name: "Drake",
external_urls: { spotify: "https://open.spotify.com/artist/3TVXtAsR1Inumwj472S9r4" },
external_urls: {
spotify: "https://open.spotify.com/artist/3TVXtAsR1Inumwj472S9r4",
},
images: [{ url: "https://i.scdn.co/image/drake.jpg" }],
followers: { total: 92000000 },
};

vi.mock("@/hooks/onboarding/useAddRosterArtist", () => ({
useAddRosterArtist: () => ({ addArtist, isAdding: false }),
vi.mock("@/hooks/useAddSpotifyArtist", () => ({
useAddSpotifyArtist: () => ({ add, isAdding: false }),
}));

vi.mock("@/hooks/useSpotifyArtistSearch", () => ({
Expand All @@ -27,8 +29,8 @@ vi.mock("@/hooks/useSpotifyArtistSearch", () => ({

describe("AddArtistForm", () => {
beforeEach(() => {
addArtist.mockClear();
addArtist.mockResolvedValue(true);
add.mockClear();
add.mockResolvedValue(true);
});

const openForm = () => {
Expand All @@ -46,13 +48,15 @@ describe("AddArtistForm", () => {
expect(screen.getByLabelText(/search spotify for an artist/i)).toBeDefined();
});

it("resolves the picked artist to its Spotify profile URL and avatar", () => {
// useAddSpotifyArtist seeds the onboarding catalog by kicking POST
// /api/valuation for the picked artist when the account has none yet.
// useAddRosterArtist never did, so an account onboarded here finished setup
// with no catalog and the valuation reward could only show its fallback
// (chat#1889 row 8).
it("adds through the hook that seeds the catalog, passing the whole Spotify result", () => {
openForm();
fireEvent.click(screen.getByRole("option", { name: /drake/i }));

expect(addArtist).toHaveBeenCalledWith("Drake", {
profileUrl: SPOTIFY_RESULT.external_urls.spotify,
image: SPOTIFY_RESULT.images[0].url,
});
expect(add).toHaveBeenCalledWith(SPOTIFY_RESULT);
});
});
19 changes: 19 additions & 0 deletions components/Onboarding/__tests__/SetupValuation.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -94,4 +94,23 @@ describe("SetupValuation", () => {
expect(getByText("hero")).toBeDefined();
expect(replace).not.toHaveBeenCalled();
});
// Seeding (chat#1889 row 8) creates the catalog seconds after the first
// artist is added, but its measurements land later — so this window is now
// routine, not theoretical. Without a terminal state the route renders a
// skeleton forever: the redirect cannot fire (there IS a catalog) and the
// hero cannot render (nothing measured yet).
it("shows a measuring state, not an endless skeleton, when the catalog has no valuation yet", () => {
catalogsResult = {
data: { catalogs: [{ id: "cat-1" }] },
isLoading: false,
isPending: false,
isError: false,
};
valuationResult = { show: false };

const { getByText } = render(<SetupValuation />);

expect(getByText(/measuring your catalog/i)).toBeDefined();
expect(replace).not.toHaveBeenCalled();
});
});
93 changes: 0 additions & 93 deletions hooks/onboarding/__tests__/useAddRosterArtist.test.tsx

This file was deleted.

84 changes: 0 additions & 84 deletions hooks/onboarding/useAddRosterArtist.ts

This file was deleted.

5 changes: 3 additions & 2 deletions hooks/useAddSpotifyArtist.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ import type { SpotifyArtistSearchResult } from "@/types/spotify";

/**
* Adds a Spotify-searched artist to the roster and refreshes + selects it.
* Wraps `addSpotifyArtist` with auth + the shared roster (ArtistProvider),
* mirroring `useAddRosterArtist`.
* Wraps `addSpotifyArtist` with auth + the shared roster (ArtistProvider).
* The onboarding roster step adds through this hook too, so a first artist
* added during setup seeds the catalog the same way (chat#1889 row 8).
*
* On the **first** artist add (the account has no catalog yet) it also
* fire-and-forget kicks `POST /api/valuation` for that artist's Spotify id —
Expand Down
12 changes: 12 additions & 0 deletions lib/artists/__tests__/addSpotifyArtist.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,16 @@ describe("addSpotifyArtist", () => {
profileUrls: { SPOTIFY: spotifyArtist.external_urls.spotify },
});
});

// POST /api/artists has already succeeded by the time enrichment runs, so a
// failing PATCH must not fail the whole add: the caller would report failure
// over an artist that exists, and picking again creates a duplicate
// (chat#1889, same defect fixed in useAddRosterArtist).
it("returns the created artist when enrichment fails", async () => {
vi.mocked(saveArtist).mockRejectedValue(new Error("Forbidden"));

const result = await addSpotifyArtist(token, spotifyArtist, "org-1");

expect(result.account_id).toBe("acc-1");
});
});
Loading
Loading