-
Notifications
You must be signed in to change notification settings - Fork 19
fix(artists): Add New Artist opens a shared Spotify search — kills the /artists loop (chat#1867) #1878
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(artists): Add New Artist opens a shared Spotify search — kills the /artists loop (chat#1867) #1878
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| "use client"; | ||
|
|
||
| import { | ||
| Dialog, | ||
| DialogContent, | ||
| DialogDescription, | ||
| DialogHeader, | ||
| DialogTitle, | ||
| } from "@/components/ui/dialog"; | ||
| import { useArtistProvider } from "@/providers/ArtistProvider"; | ||
| import { useAddSpotifyArtist } from "@/hooks/useAddSpotifyArtist"; | ||
| import SpotifyArtistSearch from "./SpotifyArtistSearch"; | ||
| import type { SpotifyArtistSearchResult } from "@/types/spotify"; | ||
|
|
||
| /** | ||
| * Global "Add New Artist" dialog: search Spotify -> pick -> add to the roster. | ||
| * Opened by `toggleCreation` (Artists page, sidebar, header), replacing the old | ||
| * `/?q=create a new artist` redirect that dead-ended on the onboarding sequence. | ||
| */ | ||
| const AddArtistDialog = () => { | ||
| const { isCreationOpen, closeCreation } = useArtistProvider(); | ||
| const { add, isAdding } = useAddSpotifyArtist(); | ||
|
|
||
| const handleSelect = async (artist: SpotifyArtistSearchResult) => { | ||
| const added = await add(artist); | ||
| if (added) closeCreation(); | ||
| }; | ||
|
|
||
| return ( | ||
| <Dialog | ||
| open={Boolean(isCreationOpen)} | ||
| onOpenChange={(open) => { | ||
| if (!open) closeCreation(); | ||
| }} | ||
| > | ||
| <DialogContent> | ||
| <DialogHeader> | ||
| <DialogTitle>Add a new artist</DialogTitle> | ||
| <DialogDescription> | ||
| Search Spotify and pick the artist to add them to your roster. | ||
| </DialogDescription> | ||
| </DialogHeader> | ||
| <SpotifyArtistSearch | ||
| onSelect={handleSelect} | ||
| isBusy={isAdding} | ||
| autoFocus | ||
| /> | ||
| </DialogContent> | ||
| </Dialog> | ||
| ); | ||
| }; | ||
|
|
||
| export default AddArtistDialog; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| "use client"; | ||
|
|
||
| import { useState } from "react"; | ||
| import { Loader, Search } from "lucide-react"; | ||
| import { Input } from "@/components/ui/input"; | ||
| import { useSpotifyArtistSearch } from "@/hooks/useSpotifyArtistSearch"; | ||
| import formatFollowerCount from "@/lib/utils/formatFollowerCount"; | ||
| import type { SpotifyArtistSearchResult } from "@/types/spotify"; | ||
|
|
||
| interface SpotifyArtistSearchProps { | ||
| onSelect: (artist: SpotifyArtistSearchResult) => void; | ||
| /** Disables input + results while an add is in flight. */ | ||
| isBusy?: boolean; | ||
| autoFocus?: boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Shared Spotify artist typeahead: type -> see matches (avatar · name · | ||
| * followers) -> pick. Reused by the "Add New Artist" dialog and (next slice) | ||
| * social enrichment, so it only searches and reports the choice — the caller | ||
| * owns what happens on select. | ||
| */ | ||
| const SpotifyArtistSearch = ({ | ||
| onSelect, | ||
| isBusy, | ||
| autoFocus, | ||
| }: SpotifyArtistSearchProps) => { | ||
| const [query, setQuery] = useState(""); | ||
| const { results, isSearching } = useSpotifyArtistSearch(query); | ||
| const hasQuery = query.trim().length >= 2; | ||
|
|
||
| return ( | ||
| <div className="flex flex-col gap-3"> | ||
| <div className="relative"> | ||
| <Search className="absolute left-3 top-1/2 -translate-y-1/2 size-4 text-muted-foreground" /> | ||
| <Input | ||
| autoFocus={autoFocus} | ||
| value={query} | ||
| onChange={(e) => setQuery(e.target.value)} | ||
| placeholder="Search Spotify for an artist" | ||
| className="pl-9" | ||
| aria-label="Search Spotify for an artist" | ||
| disabled={isBusy} | ||
| /> | ||
| </div> | ||
|
|
||
| <div | ||
| className="flex max-h-72 flex-col gap-1 overflow-y-auto" | ||
| role="listbox" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: This announces a listbox to assistive technology without implementing listbox keyboard/selection behavior, while also placing loading and empty messages inside the option container. Either implement the listbox interaction model or use an ordinary results container with keyboard-accessible buttons and status messages outside it. Prompt for AI agents |
||
| aria-label="Spotify artist results" | ||
| > | ||
| {isSearching && results.length === 0 && ( | ||
| <p className="flex items-center gap-2 px-1 py-2 text-sm text-muted-foreground"> | ||
| <Loader className="size-4 animate-spin" /> Searching... | ||
| </p> | ||
| )} | ||
| {!isSearching && hasQuery && results.length === 0 && ( | ||
| <p className="px-1 py-2 text-sm text-muted-foreground"> | ||
| No artists found. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Network or Spotify API failures are presented as “No artists found,” which misleads users and gives them no recovery path. Exposing a search error from Prompt for AI agents |
||
| </p> | ||
| )} | ||
| {results.map((artist) => ( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Typing a new search can leave results for the previous query displayed and clickable until the new request finishes. Clearing results when a query changes, or hiding them while Prompt for AI agents |
||
| <button | ||
| key={artist.id} | ||
| type="button" | ||
| role="option" | ||
| aria-selected={false} | ||
| disabled={isBusy} | ||
| onClick={() => onSelect(artist)} | ||
| className="flex items-center gap-3 rounded-lg p-2 text-left hover:bg-muted disabled:opacity-50" | ||
| > | ||
| {artist.images?.[0]?.url ? ( | ||
| // eslint-disable-next-line @next/next/no-img-element | ||
| <img | ||
| src={artist.images[0].url} | ||
| alt="" | ||
| className="size-10 shrink-0 rounded-full object-cover" | ||
| /> | ||
| ) : ( | ||
| <div className="size-10 shrink-0 rounded-full bg-muted" /> | ||
| )} | ||
| <div className="min-w-0"> | ||
| <p className="truncate text-sm font-medium text-foreground"> | ||
| {artist.name} | ||
| </p> | ||
| {typeof artist.followers?.total === "number" && ( | ||
| <p className="text-xs text-muted-foreground"> | ||
| {formatFollowerCount(artist.followers.total)} followers | ||
| </p> | ||
| )} | ||
| </div> | ||
| </button> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| export default SpotifyArtistSearch; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| "use client"; | ||
|
|
||
| import { useState } from "react"; | ||
| import { usePrivy } from "@privy-io/react-auth"; | ||
| import { toast } from "sonner"; | ||
| import { addSpotifyArtist } from "@/lib/artists/addSpotifyArtist"; | ||
| import { useArtistProvider } from "@/providers/ArtistProvider"; | ||
| import { useOrganization } from "@/providers/OrganizationProvider"; | ||
| 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`. | ||
| */ | ||
| export function useAddSpotifyArtist() { | ||
| const { getAccessToken } = usePrivy(); | ||
| const { getArtists } = useArtistProvider(); | ||
| const { selectedOrgId } = useOrganization(); | ||
| const [isAdding, setIsAdding] = useState(false); | ||
|
|
||
| const add = async (artist: SpotifyArtistSearchResult): Promise<boolean> => { | ||
| setIsAdding(true); | ||
| try { | ||
| const accessToken = await getAccessToken(); | ||
| if (!accessToken) { | ||
| throw new Error("Please sign in to add an artist"); | ||
| } | ||
| const created = await addSpotifyArtist( | ||
| accessToken, | ||
| artist, | ||
| selectedOrgId, | ||
| ); | ||
| await getArtists(created.account_id); | ||
| toast.success(`${artist.name} added to your roster`); | ||
| return true; | ||
| } catch (error) { | ||
| toast.error( | ||
| error instanceof Error ? error.message : "Failed to add artist", | ||
| ); | ||
| return false; | ||
| } finally { | ||
| setIsAdding(false); | ||
| } | ||
| }; | ||
|
|
||
| return { add, isAdding }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,24 +1,28 @@ | ||
| import { useUserProvider } from "@/providers/UserProvder"; | ||
| import { ArtistRecord } from "@/types/Artist"; | ||
| import { SETTING_MODE } from "@/types/Setting"; | ||
| import { useRouter } from "next/navigation"; | ||
| import { Dispatch, SetStateAction, useState } from "react"; | ||
|
|
||
| const useArtistMode = ( | ||
| clearParams: () => void, | ||
| setEditableArtist: Dispatch<SetStateAction<ArtistRecord | null>> | ||
| setEditableArtist: Dispatch<SetStateAction<ArtistRecord | null>>, | ||
| ) => { | ||
| const [settingMode, setSettingMode] = useState(SETTING_MODE.UPDATE); | ||
| const [isOpenSettingModal, setIsOpenSettingModal] = useState(false); | ||
| const [isCreationOpen, setIsCreationOpen] = useState(false); | ||
| const { email } = useUserProvider(); | ||
| const { push } = useRouter(); | ||
|
|
||
| // Opens the shared Spotify-search dialog (AddArtistDialog). Previously this | ||
| // pushed `/?q=create a new artist`, which the onboarding router now | ||
| // intercepts for incomplete accounts — dead-ending in an /artists loop. | ||
| const toggleCreation = () => { | ||
| clearParams(); | ||
| if (!email) return; | ||
| push("/?q=create a new artist"); | ||
| clearParams(); | ||
| setIsCreationOpen(true); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When this path is reached from Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The new dialog path doesn't reset the legacy Prompt for AI agents |
||
| }; | ||
|
Comment on lines
18
to
22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check whether callers of toggleCreation gate/disable the trigger for incomplete accounts
rg -n -B3 -A3 'toggleCreation' --type=tsx --type=tsRepository: recoupable/chat Length of output: 181 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n== toggleCreation usages ==\n'
rg -n -B3 -A5 'toggleCreation' . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'
printf '\n== artist mode hook file ==\n'
git ls-files 'hooks/useArtistMode.tsx' 'hooks/*Artist*' 'components/*Artist*' 'app/*Artist*' 'pages/*Artist*' | sed 's#^`#-` #'Repository: recoupable/chat Length of output: 9941 🏁 Script executed: #!/bin/bash
set -euo pipefail
for f in hooks/useArtistMode.tsx components/Header/Header.tsx components/Artists/Artists.tsx components/SideMenu/SideMenu.tsx components/CreateWorkspaceModal.tsx; do
echo "===== $f ====="
wc -l "$f"
echo "--- outline ---"
ast-grep outline "$f" --view expanded | sed -n '1,220p'
echo
doneRepository: recoupable/chat Length of output: 1027 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n== hooks/useArtistMode.tsx around toggleCreation ==\n'
sed -n '1,120p' hooks/useArtistMode.tsx
printf '\n== components/Header/Header.tsx relevant section ==\n'
sed -n '1,140p' components/Header/Header.tsx
printf '\n== components/Artists/Artists.tsx relevant section ==\n'
sed -n '1,120p' components/Artists/Artists.tsx
printf '\n== components/SideMenu/SideMenu.tsx relevant section ==\n'
sed -n '1,140p' components/SideMenu/SideMenu.tsx
printf '\n== components/CreateWorkspaceModal.tsx relevant section ==\n'
sed -n '1,120p' components/CreateWorkspaceModal.tsxRepository: recoupable/chat Length of output: 14026 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n== email-related gating near add-artist actions ==\n'
rg -n -B4 -A8 'email|toggleCreation|Add New Artist|create a new artist|isIncomplete|incomplete' \
hooks/useArtistMode.tsx \
components/Header/Header.tsx \
components/Artists/Artists.tsx \
components/SideMenu/SideMenu.tsx \
components/CreateWorkspaceModal.tsx \
--glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'Repository: recoupable/chat Length of output: 8870 Surface a fallback for incomplete accounts 🤖 Prompt for AI Agents |
||
|
|
||
| const closeCreation = () => setIsCreationOpen(false); | ||
|
|
||
| const toggleUpdate = (artist: ArtistRecord) => { | ||
| setSettingMode(SETTING_MODE.UPDATE); | ||
| setEditableArtist(artist); | ||
|
|
@@ -32,6 +36,8 @@ const useArtistMode = ( | |
| toggleUpdate, | ||
| toggleSettingModal, | ||
| toggleCreation, | ||
| closeCreation, | ||
| isCreationOpen, | ||
| settingMode, | ||
| setSettingMode, | ||
| isOpenSettingModal, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| "use client"; | ||
|
|
||
| import { useEffect, useState } from "react"; | ||
| import { getClientApiBaseUrl } from "@/lib/api/getClientApiBaseUrl"; | ||
| import type { | ||
| SpotifyArtistSearchResult, | ||
| SpotifySearchResponse, | ||
| } from "@/types/spotify"; | ||
|
|
||
| interface UseSpotifyArtistSearchResult { | ||
| results: SpotifyArtistSearchResult[]; | ||
| isSearching: boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Debounced Spotify artist typeahead against the existing public | ||
| * `GET /api/spotify/search?type=artist` endpoint. Aborts the in-flight | ||
| * request when the query changes so results never arrive out of order. | ||
| */ | ||
| export function useSpotifyArtistSearch( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The hook is difficult to test and maintain because its request/lifecycle logic is concentrated in a function over twice the repository's 20-line limit. Extracting the fetch/parse callback or lifecycle helper would keep the hook focused. Prompt for AI agents |
||
| query: string, | ||
| ): UseSpotifyArtistSearchResult { | ||
| const [results, setResults] = useState<SpotifyArtistSearchResult[]>([]); | ||
| const [isSearching, setIsSearching] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| const trimmed = query.trim(); | ||
| if (trimmed.length < 2) { | ||
| setResults([]); | ||
| setIsSearching(false); | ||
| return; | ||
| } | ||
|
|
||
| setIsSearching(true); | ||
| const controller = new AbortController(); | ||
| const timer = setTimeout(async () => { | ||
| try { | ||
| const res = await fetch( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The Spotify search fetch doesn't check Prompt for AI agents |
||
| `${getClientApiBaseUrl()}/api/spotify/search?q=${encodeURIComponent( | ||
| trimmed, | ||
| )}&type=artist`, | ||
| { signal: controller.signal }, | ||
| ); | ||
| const data: SpotifySearchResponse = await res.json(); | ||
| setResults(data.artists?.items ?? []); | ||
| } catch { | ||
| if (!controller.signal.aborted) setResults([]); | ||
| } finally { | ||
| if (!controller.signal.aborted) setIsSearching(false); | ||
| } | ||
| }, 300); | ||
|
|
||
| return () => { | ||
| controller.abort(); | ||
| clearTimeout(timer); | ||
| }; | ||
| }, [query]); | ||
|
|
||
| return { results, isSearching }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { addSpotifyArtist } from "@/lib/artists/addSpotifyArtist"; | ||
| import { createRosterArtist } from "@/lib/artists/createRosterArtist"; | ||
| import saveArtist from "@/lib/saveArtist"; | ||
| import type { SpotifyArtistSearchResult } from "@/types/spotify"; | ||
|
|
||
| vi.mock("@/lib/artists/createRosterArtist", () => ({ | ||
| createRosterArtist: vi.fn(), | ||
| })); | ||
| vi.mock("@/lib/saveArtist", () => ({ default: vi.fn() })); | ||
|
|
||
| const spotifyArtist: SpotifyArtistSearchResult = { | ||
| id: "0xPoVNPnxIIUS1vrxAYV00", | ||
| name: "Del Water Gap", | ||
| type: "artist", | ||
| uri: "spotify:artist:0xPoVNPnxIIUS1vrxAYV00", | ||
| external_urls: { | ||
| spotify: "https://open.spotify.com/artist/0xPoVNPnxIIUS1vrxAYV00", | ||
| }, | ||
| images: [{ url: "https://i.scdn.co/image/big", height: 640, width: 640 }], | ||
| popularity: 60, | ||
| genres: [], | ||
| followers: { href: null, total: 317952 }, | ||
| }; | ||
|
|
||
| describe("addSpotifyArtist", () => { | ||
| const token = "tok"; | ||
| const created = { id: "acc-1", account_id: "acc-1", name: "Del Water Gap" }; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| vi.mocked(createRosterArtist).mockResolvedValue(created as never); | ||
| vi.mocked(saveArtist).mockResolvedValue({ | ||
| artist: { ...created, image: "https://i.scdn.co/image/big" }, | ||
| } as never); | ||
| }); | ||
|
|
||
| it("creates the artist by name then links its Spotify image + profile URL", async () => { | ||
| const result = await addSpotifyArtist(token, spotifyArtist, "org-1"); | ||
|
|
||
| expect(createRosterArtist).toHaveBeenCalledWith( | ||
| token, | ||
| "Del Water Gap", | ||
| "org-1", | ||
| ); | ||
| expect(saveArtist).toHaveBeenCalledWith(token, "acc-1", { | ||
| image: "https://i.scdn.co/image/big", | ||
| profileUrls: { SPOTIFY: spotifyArtist.external_urls.spotify }, | ||
| }); | ||
| expect(result.account_id).toBe("acc-1"); | ||
| }); | ||
|
|
||
| it("omits the image when the Spotify result has none", async () => { | ||
| await addSpotifyArtist(token, { ...spotifyArtist, images: [] }); | ||
| expect(saveArtist).toHaveBeenCalledWith(token, "acc-1", { | ||
| profileUrls: { SPOTIFY: spotifyArtist.external_urls.spotify }, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: The search input acts as a combobox but lacks the ARIA combobox pattern. Screen reader users won't know that typing yields a selectable list of Spotify artists below the input. Add
role="combobox",aria-expanded(reflecting whether the listbox is visible), andaria-controlspointing to the results container'sidto align with the WAI-ARIA combobox pattern and improve discoverability for assistive technology users.Prompt for AI agents