-
Notifications
You must be signed in to change notification settings - Fork 9
fix: self-mapping album captures — resolve ISRCs for unmapped tracks #664
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
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,77 @@ | ||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
| import { mapUnmappedAlbumTracks } from "../mapUnmappedAlbumTracks"; | ||
|
|
||
| import generateAccessToken from "@/lib/spotify/generateAccessToken"; | ||
| import getTracks from "@/lib/spotify/getTracks"; | ||
| import { upsertSongs } from "@/lib/supabase/songs/upsertSongs"; | ||
| import { upsertSongIdentifiers } from "@/lib/supabase/song_identifiers/upsertSongIdentifiers"; | ||
|
|
||
| vi.mock("@/lib/spotify/generateAccessToken", () => ({ default: vi.fn() })); | ||
| vi.mock("@/lib/spotify/getTracks", () => ({ default: vi.fn() })); | ||
| vi.mock("@/lib/supabase/songs/upsertSongs", () => ({ upsertSongs: vi.fn() })); | ||
| vi.mock("@/lib/supabase/song_identifiers/upsertSongIdentifiers", () => ({ | ||
| upsertSongIdentifiers: vi.fn(), | ||
| })); | ||
|
|
||
| const ALBUMS = [ | ||
| { | ||
| id: "album_1", | ||
| name: "K.I.D.S. (Deluxe)", | ||
| tracks: [ | ||
| { id: "t_mapped", name: "The Spins", streamCount: 1 }, | ||
| { id: "t_new", name: "Nikes on My Feet", streamCount: 2 }, | ||
| { id: "t_noisrc", name: "Interlude", streamCount: 3 }, | ||
| ], | ||
| }, | ||
| ]; | ||
|
|
||
| describe("mapUnmappedAlbumTracks", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| vi.mocked(generateAccessToken).mockResolvedValue({ access_token: "tok" } as never); | ||
| vi.mocked(upsertSongs).mockResolvedValue([] as never); | ||
| }); | ||
|
|
||
| it("resolves ISRCs for unmapped tracks, upserts songs + identifiers, returns new mappings", async () => { | ||
| vi.mocked(getTracks).mockResolvedValue({ | ||
| tracks: [ | ||
| { id: "t_new", name: "Nikes on My Feet", external_ids: { isrc: "ISRC_NIKES" } }, | ||
| { id: "t_noisrc", name: "Interlude", external_ids: {} }, | ||
| ], | ||
| error: null, | ||
| } as never); | ||
|
|
||
| const mapped = await mapUnmappedAlbumTracks(ALBUMS, new Set(["t_mapped"])); | ||
|
|
||
| expect(getTracks).toHaveBeenCalledWith({ ids: ["t_new", "t_noisrc"], accessToken: "tok" }); | ||
| expect(upsertSongs).toHaveBeenCalledWith([ | ||
| { isrc: "ISRC_NIKES", name: "Nikes on My Feet", album: "K.I.D.S. (Deluxe)" }, | ||
| ]); | ||
| expect(upsertSongIdentifiers).toHaveBeenCalledWith([ | ||
| { song: "ISRC_NIKES", platform: "spotify", identifier_type: "track_id", value: "t_new" }, | ||
| { song: "ISRC_NIKES", platform: "spotify", identifier_type: "album_id", value: "album_1" }, | ||
| ]); | ||
| expect([...mapped.entries()]).toEqual([["t_new", "ISRC_NIKES"]]); | ||
| }); | ||
|
|
||
| it("returns an empty map without Spotify calls when everything is mapped", async () => { | ||
| const mapped = await mapUnmappedAlbumTracks(ALBUMS, new Set(["t_mapped", "t_new", "t_noisrc"])); | ||
|
|
||
| expect(generateAccessToken).not.toHaveBeenCalled(); | ||
| expect(mapped.size).toBe(0); | ||
| }); | ||
|
|
||
| it("degrades to an empty map (no throw) when Spotify auth fails — capture proceeds for already-mapped tracks", async () => { | ||
| const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); | ||
| vi.mocked(generateAccessToken).mockResolvedValue({ | ||
| access_token: null, | ||
| error: new Error("down"), | ||
| } as never); | ||
|
|
||
| const mapped = await mapUnmappedAlbumTracks(ALBUMS, new Set()); | ||
|
|
||
| expect(mapped.size).toBe(0); | ||
| expect(consoleError).toHaveBeenCalled(); | ||
| consoleError.mockRestore(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| import generateAccessToken from "@/lib/spotify/generateAccessToken"; | ||
| import getTracks from "@/lib/spotify/getTracks"; | ||
| import { upsertSongs } from "@/lib/supabase/songs/upsertSongs"; | ||
| import { upsertSongIdentifiers } from "@/lib/supabase/song_identifiers/upsertSongIdentifiers"; | ||
| import { SpotifyAlbumPlayCounts } from "@/lib/apify/spotify/fetchSpotifyAlbumPlayCounts"; | ||
|
|
||
| /** | ||
| * Self-mapping bootstrap (chat#1794): resolve ISRCs for actor tracks that have | ||
| * no identifier mapping yet — batch Spotify `/v1/tracks` lookup — and upsert | ||
| * the `songs` rows plus track_id/album_id mappings. Failures degrade to an | ||
| * empty map (logged): capture proceeds for already-mapped tracks and the next | ||
| * snapshot retries the rest. | ||
| * | ||
| * @param albums - Parsed actor album items (with album `id`) | ||
| * @param mappedTrackIds - Track ids that already have mappings | ||
| * @returns Newly created trackId → ISRC mappings | ||
| */ | ||
| export async function mapUnmappedAlbumTracks( | ||
| albums: SpotifyAlbumPlayCounts[], | ||
| mappedTrackIds: Set<string>, | ||
| ): Promise<Map<string, string>> { | ||
| const unmapped = albums.flatMap(album => | ||
| (album.tracks ?? []) | ||
| .filter(track => !mappedTrackIds.has(track.id)) | ||
| .map(track => ({ trackId: track.id, albumId: album.id, albumName: album.name })), | ||
| ); | ||
| if (unmapped.length === 0) return new Map(); | ||
|
|
||
| try { | ||
| const { access_token, error: tokenError } = await generateAccessToken(); | ||
| if (!access_token) throw tokenError ?? new Error("No Spotify access token"); | ||
|
|
||
| const { tracks, error } = await getTracks({ | ||
| ids: unmapped.map(u => u.trackId), | ||
| accessToken: access_token, | ||
| }); | ||
| if (!tracks) throw error ?? new Error("Spotify tracks lookup failed"); | ||
|
|
||
| const contextByTrackId = new Map(unmapped.map(u => [u.trackId, u])); | ||
| const resolved = tracks.flatMap(track => { | ||
| const isrc = track.external_ids?.isrc; | ||
| const context = contextByTrackId.get(track.id); | ||
| if (!isrc || !context) return []; | ||
| return [ | ||
| { | ||
| trackId: track.id, | ||
| isrc, | ||
| name: track.name, | ||
| albumId: context.albumId, | ||
| albumName: context.albumName, | ||
| }, | ||
| ]; | ||
| }); | ||
| if (resolved.length === 0) return new Map(); | ||
|
|
||
| await upsertSongs( | ||
| resolved.map(r => ({ isrc: r.isrc, name: r.name ?? null, album: r.albumName ?? null })), | ||
|
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: Potential duplicate ISRC rows are sent to Prompt for AI agents |
||
| ); | ||
| await upsertSongIdentifiers( | ||
| resolved.flatMap(r => [ | ||
| { song: r.isrc, platform: "spotify", identifier_type: "track_id", value: r.trackId }, | ||
| ...(r.albumId | ||
| ? [{ song: r.isrc, platform: "spotify", identifier_type: "album_id", value: r.albumId }] | ||
| : []), | ||
| ]), | ||
| ); | ||
|
|
||
| return new Map(resolved.map(r => [r.trackId, r.isrc])); | ||
| } catch (error) { | ||
| console.error("[playcounts] identifier bootstrap failed:", error); | ||
| return new Map(); | ||
| } | ||
| } | ||
|
Comment on lines
+18
to
+73
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. 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift Function exceeds the 50-line guideline for domain functions. The
This would improve readability and testability while aligning with the Single Responsibility Principle. As per coding guidelines: "Keep functions under 50 lines" for domain functions in 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
| import getTracks from "../getTracks"; | ||
|
|
||
| const mockFetch = vi.fn(); | ||
| global.fetch = mockFetch as never; | ||
|
|
||
| describe("getTracks", () => { | ||
| beforeEach(() => vi.clearAllMocks()); | ||
|
|
||
| it("batch-fetches tracks (50 per call) and concatenates results", async () => { | ||
| const ids = Array.from({ length: 60 }, (_, i) => `t${i}`); | ||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| json: async () => ({ tracks: [{ id: "t0", external_ids: { isrc: "ISRC0" } }] }), | ||
| } as never); | ||
|
|
||
| const { tracks, error } = await getTracks({ ids, accessToken: "tok" }); | ||
|
|
||
| expect(mockFetch).toHaveBeenCalledTimes(2); | ||
| expect(mockFetch.mock.calls[0][0]).toContain( | ||
| "ids=" + encodeURIComponent(ids.slice(0, 50).join(",")), | ||
| ); | ||
| expect(error).toBeNull(); | ||
| expect(tracks).toHaveLength(2); | ||
| }); | ||
|
|
||
| it("returns an error on a failed response", async () => { | ||
| mockFetch.mockResolvedValue({ ok: false, status: 429 } as never); | ||
|
|
||
| const { tracks, error } = await getTracks({ ids: ["t1"], accessToken: "tok" }); | ||
|
|
||
| expect(tracks).toBeNull(); | ||
| expect(error).toBeInstanceOf(Error); | ||
| }); | ||
|
|
||
| it("returns [] for empty input without fetching", async () => { | ||
| const { tracks } = await getTracks({ ids: [], accessToken: "tok" }); | ||
|
|
||
| expect(mockFetch).not.toHaveBeenCalled(); | ||
| expect(tracks).toEqual([]); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| import { SpotifyTrack } from "@/types/spotify.types"; | ||
|
|
||
| const BATCH_SIZE = 50; | ||
|
|
||
| /** | ||
| * Fetch multiple tracks by id via `GET /v1/tracks` (the only public surface | ||
| * that returns `external_ids.isrc`), batched at the API's 50-id limit. | ||
| * | ||
| * @param params.ids - Spotify track ids | ||
| * @param params.accessToken - Client-credentials access token | ||
| * @returns All fetched tracks, or an error | ||
| */ | ||
| const getTracks = async ({ | ||
| ids, | ||
| accessToken, | ||
| }: { | ||
| ids: string[]; | ||
| accessToken: string; | ||
| }): Promise<{ tracks: SpotifyTrack[] | null; error: Error | null }> => { | ||
| if (ids.length === 0) return { tracks: [], error: null }; | ||
|
|
||
| try { | ||
| const tracks: SpotifyTrack[] = []; | ||
| for (let i = 0; i < ids.length; i += BATCH_SIZE) { | ||
| const batch = ids.slice(i, i + BATCH_SIZE); | ||
| const url = `https://api.spotify.com/v1/tracks?ids=${encodeURIComponent(batch.join(","))}`; | ||
| const response = await fetch(url, { | ||
|
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: Add a timeout to this external API call. Without an Prompt for AI agents |
||
| method: "GET", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${accessToken}`, | ||
| }, | ||
| }); | ||
|
Comment on lines
+27
to
+33
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. Add timeout to external API calls. The fetch request to Spotify has no timeout, which means the call could hang indefinitely if Spotify is slow or unresponsive. This blocks the request thread and degrades system availability. Consider adding a timeout using AbortController: 🛡️ Recommended fix to add timeout const getTracks = async ({
ids,
accessToken,
}: {
ids: string[];
accessToken: string;
}): Promise<{ tracks: SpotifyTrack[] | null; error: Error | null }> => {
if (ids.length === 0) return { tracks: [], error: null };
try {
const tracks: SpotifyTrack[] = [];
for (let i = 0; i < ids.length; i += BATCH_SIZE) {
const batch = ids.slice(i, i + BATCH_SIZE);
const url = `https://api.spotify.com/v1/tracks?ids=${encodeURIComponent(batch.join(","))}`;
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 10000); // 10s timeout
+
const response = await fetch(url, {
method: "GET",
headers: {
"Content-Type": "application/json",
Authorization: `Bearer ${accessToken}`,
},
+ signal: controller.signal,
});
+ clearTimeout(timeoutId);
if (!response.ok) {
return {
tracks: null,
error: new Error(`Spotify tracks request failed: ${response.status}`),
};
}
const data = await response.json();
tracks.push(...(data.tracks ?? []).filter(Boolean));
}
return { tracks, error: null };
} catch (error) {
console.error(error);
return {
tracks: null,
error: error instanceof Error ? error : new Error("Unknown error fetching tracks"),
};
}
};🤖 Prompt for AI Agents |
||
|
|
||
| if (!response.ok) { | ||
| return { | ||
| tracks: null, | ||
| error: new Error(`Spotify tracks request failed: ${response.status}`), | ||
| }; | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| tracks.push(...(data.tracks ?? []).filter(Boolean)); | ||
| } | ||
| return { tracks, error: null }; | ||
| } catch (error) { | ||
| console.error(error); | ||
| return { | ||
| tracks: null, | ||
| error: error instanceof Error ? error : new Error("Unknown error fetching tracks"), | ||
| }; | ||
| } | ||
| }; | ||
|
|
||
| export default getTracks; | ||
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: Function is 56 lines, exceeding the project's 50-line guideline for domain functions in
lib/**/*.ts. Consider extractingresolveTracksFromSpotify(token + fetch) andpersistResolvedMappings(upsert songs + identifiers) to improve readability and testability.Prompt for AI agents