feat(catalogs): artist_account_id filter + no-cap pagination on GET /api/catalogs/measurements#763
feat(catalogs): artist_account_id filter + no-cap pagination on GET /api/catalogs/measurements#763sweetmantech wants to merge 1 commit into
Conversation
…api/catalogs/measurements (chat#1850) Two fixes on the same read path (v2 of api#757): - FIX the silent 1,000-row cap: the catalog tracklist, the measurement series, and the artist-link read all paginate to exhaustion with .range() loops (and the measurements IN clause is chunked at 500 ISRCs to stay under URL limits) BEFORE dedupe/derivation, so total_streams and the valuation band cover every measured song. Live repro was a 2,679-song catalog valued 3.4x low off exactly 1,000 rows. - ADD optional artist_account_id (uuid, 400 on malformed): scopes measurements + valuation to the catalog's songs linked to that artist account via song_artists (catalog_songs ∩ song_artists). The response echoes the applied filter as artist_account_id (null when unfiltered) so clients can verify the scope before rendering artist-labeled money. Per the docs contract in recoupable/docs#267. TDD: RED confirmed before GREEN for every unit; full suite 3,945 tests, tsc 0 new errors, lint clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 12 files
Confidence score: 2/5
- In
lib/supabase/catalog_songs/selectCatalogSongTitles.tsandlib/supabase/song_artists/selectSongArtistIsrcs.ts, pagination errors on later pages currently drop already-fetched results and return[], which is indistinguishable from a truly empty catalog and can silently undercount downstream data — preserve partial results or propagate an explicit error state before merging. - In
lib/supabase/song_measurements/selectAllSongMeasurements.ts, any page/chunk failure can causeGET /api/catalogs/measurementsto look like zero data/zero valuation, creating a concrete user-facing misreporting risk — return a typed failure (or throw) so callers can surface an error instead of treating it as empty data. - In
lib/catalog/getCatalogMeasurementsHandler.ts, artist-scoped valuation mixes artist-filtered stream totals with catalog-wide age, which can skew outputs for scoped views — derivecatalog_age_yearsfrom the same scoped song set before merge to keep calculations consistent. - Tests in
lib/supabase/song_measurements/__tests__/selectAllSongMeasurements.test.tsmiss coverage for the secondaryidsort assertion and for verifyingconsole.erroremission, so regressions in deterministic pagination/error observability may slip through — tighten these assertions to de-risk future changes.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/supabase/catalog_songs/selectCatalogSongTitles.ts">
<violation number="1" location="lib/supabase/catalog_songs/selectCatalogSongTitles.ts:19">
P1: Pagination error on a later page silently discards data accumulated from earlier pages. If page 1 succeeds (fetching up to 1,000 rows) and page 2 errors, the function returns `[]` — the same value as an empty catalog. A transient failure on any page beyond the first produces a false empty result that the caller cannot distinguish from a genuinely empty catalog, leading to silent data loss in production for large catalogs. Consider either: (a) returning the partial data with a flag or log indicating truncation, (b) retrying the failed page before giving up, or (c) accumulating `all` through the loop and only returning `[]` when no pages at all were fetched (i.e., page 1 itself failed).</violation>
</file>
<file name="lib/supabase/song_measurements/selectAllSongMeasurements.ts">
<violation number="1" location="lib/supabase/song_measurements/selectAllSongMeasurements.ts:48">
P2: GET /api/catalogs/measurements can report an empty catalog/zero valuation when any measurement page or chunk fails. Since this helper returns [] on `error`, the handler cannot distinguish a real no-data result from a failed exhaustive read; throwing here would let the existing handler return its 500 path.</violation>
</file>
<file name="lib/supabase/song_measurements/__tests__/selectAllSongMeasurements.test.ts">
<violation number="1" location="lib/supabase/song_measurements/__tests__/selectAllSongMeasurements.test.ts:57">
P2: Missing assertion for the secondary sort column `id`. The implementation uses `.order("id", { ascending: false })` to ensure deterministic pagination, but the test for the small-batch path only asserts `.order("captured_at", ...)` was called. If the `id` sort were accidentally removed, pagination could produce unstable pages (rows shifting between pages, silently duplicated or dropped during pagination).</violation>
<violation number="2" location="lib/supabase/song_measurements/__tests__/selectAllSongMeasurements.test.ts:100">
P3: The error test doesn't verify `console.error` was called. The test mocks `console.error` and asserts the return value, but never asserts the log was actually emitted. If a future refactor accidentally drops the `console.error` call, this test won't catch it. Add `expect(consoleError).toHaveBeenCalledWith("Error fetching song_measurements:", { message: "boom" })` before the restore.</violation>
</file>
<file name="lib/catalog/getCatalogMeasurementsHandler.ts">
<violation number="1" location="lib/catalog/getCatalogMeasurementsHandler.ts:48">
P2: Artist-scoped valuation can be skewed because the stream total is filtered to the artist while `catalog_age_years` still comes from the whole catalog. Consider resolving the earliest release date from the scoped song set (or a scoped album/source lookup) when `artist_account_id` is provided.</violation>
</file>
<file name="lib/supabase/song_artists/selectSongArtistIsrcs.ts">
<violation number="1" location="lib/supabase/song_artists/selectSongArtistIsrcs.ts:17">
P1: Same partial-failure data loss as `selectCatalogSongTitles.ts`. If the first page of `song_artists` succeeds but a later page errors, the accumulated ISRCs from earlier pages are discarded and `[]` is returned. This would cause the downstream catalog-measurement scope resolution to appear empty (no songs for the artist) when in reality data was partially fetched and then lost to a transient error. Consider applying the same fix strategy as the sibling function for consistency.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as API Client
participant Handler as getCatalogMeasurementsHandler
participant Validator as validateGetCatalogMeasurementsQuery
participant Auth as validateAuthContext
participant DBCatalog as selectAccountCatalog
participant Resolver as resolveCatalogSongsInScope
participant DBSongs as selectCatalogSongTitles
participant DBArtists as selectSongArtistIsrcs
participant DBMeas as selectAllSongMeasurements
Note over Handler: NEW: pagination + artist filter + response echo
Client->>Handler: GET /api/catalogs/measurements?catalogId=&artist_account_id=
Handler->>Validator: validate query params
Validator-->>Handler: {catalogId, artist_account_id} or 400
alt malformed artist_account_id (not uuid)
Handler-->>Client: 400 error
end
Handler->>Auth: validateAuthContext(request)
Auth-->>Handler: {accountId}
Handler->>DBCatalog: selectAccountCatalog({accountId, catalogId})
DBCatalog-->>Handler: catalog link or null
alt catalog not found or forbidden
Handler-->>Client: 404
end
Note over Handler: NEW: resolve scope (whole catalog vs artist filtered)
Handler->>Resolver: resolveCatalogSongsInScope({catalogId, artistAccountId})
Resolver->>DBSongs: CHANGED: selectCatalogSongTitles(catalogId)
Note over DBSongs: Paginates past 1,000-row default. Loops .range() until <1000 rows.
DBSongs-->>Resolver: all song {isrc, title} pairs
alt artistAccountId provided
Resolver->>DBArtists: NEW: selectSongArtistIsrcs(artistAccountId)
Note over DBArtists: Also paginates with .range() to exhaustion.
DBArtists-->>Resolver: ISRCs linked to artist
Note over Resolver: Intersect catalog songs with artist ISRCs
end
Resolver-->>Handler: songs in scope (possibly empty)
Note over Handler: CHANGED: empty scope skips measurement fetch
alt songs empty
Handler->>Handler: derive zero totals, zero valuation band
else songs non-empty
Handler->>DBMeas: NEW: selectAllSongMeasurements(songs, platform, metric)
Note over DBMeas: Chunks ISRC list (500 per chunk).<br/>Each chunk paginated with .range() to exhaustion.<br/>Order: captured_at descending, id descending.
DBMeas-->>Handler: all measurement rows
Handler->>Handler: latest per ISRC, total_streams, catalog_age_years, valuation band
end
Note over Handler: NEW: echo applied filter in response
Handler-->>Client: {measurements, total_streams, valuation, artist_account_id, catalog_age_years}
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| console.error("Error fetching catalog_songs:", error); | ||
| return []; | ||
| } | ||
| for (let from = 0; ; from += PAGE_SIZE) { |
There was a problem hiding this comment.
P1: Pagination error on a later page silently discards data accumulated from earlier pages. If page 1 succeeds (fetching up to 1,000 rows) and page 2 errors, the function returns [] — the same value as an empty catalog. A transient failure on any page beyond the first produces a false empty result that the caller cannot distinguish from a genuinely empty catalog, leading to silent data loss in production for large catalogs. Consider either: (a) returning the partial data with a flag or log indicating truncation, (b) retrying the failed page before giving up, or (c) accumulating all through the loop and only returning [] when no pages at all were fetched (i.e., page 1 itself failed).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/catalog_songs/selectCatalogSongTitles.ts, line 19:
<comment>Pagination error on a later page silently discards data accumulated from earlier pages. If page 1 succeeds (fetching up to 1,000 rows) and page 2 errors, the function returns `[]` — the same value as an empty catalog. A transient failure on any page beyond the first produces a false empty result that the caller cannot distinguish from a genuinely empty catalog, leading to silent data loss in production for large catalogs. Consider either: (a) returning the partial data with a flag or log indicating truncation, (b) retrying the failed page before giving up, or (c) accumulating `all` through the loop and only returning `[]` when no pages at all were fetched (i.e., page 1 itself failed).</comment>
<file context>
@@ -2,24 +2,35 @@ import supabase from "../serverClient";
- console.error("Error fetching catalog_songs:", error);
- return [];
- }
+ for (let from = 0; ; from += PAGE_SIZE) {
+ const { data, error } = await supabase
+ .from("catalog_songs")
</file context>
| export async function selectSongArtistIsrcs(artistAccountId: string): Promise<string[]> { | ||
| const all: string[] = []; | ||
|
|
||
| for (let from = 0; ; from += PAGE_SIZE) { |
There was a problem hiding this comment.
P1: Same partial-failure data loss as selectCatalogSongTitles.ts. If the first page of song_artists succeeds but a later page errors, the accumulated ISRCs from earlier pages are discarded and [] is returned. This would cause the downstream catalog-measurement scope resolution to appear empty (no songs for the artist) when in reality data was partially fetched and then lost to a transient error. Consider applying the same fix strategy as the sibling function for consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/song_artists/selectSongArtistIsrcs.ts, line 17:
<comment>Same partial-failure data loss as `selectCatalogSongTitles.ts`. If the first page of `song_artists` succeeds but a later page errors, the accumulated ISRCs from earlier pages are discarded and `[]` is returned. This would cause the downstream catalog-measurement scope resolution to appear empty (no songs for the artist) when in reality data was partially fetched and then lost to a transient error. Consider applying the same fix strategy as the sibling function for consistency.</comment>
<file context>
@@ -0,0 +1,34 @@
+export async function selectSongArtistIsrcs(artistAccountId: string): Promise<string[]> {
+ const all: string[] = [];
+
+ for (let from = 0; ; from += PAGE_SIZE) {
+ const { data, error } = await supabase
+ .from("song_artists")
</file context>
|
|
||
| if (error) { | ||
| console.error("Error fetching song_measurements:", error); | ||
| return []; |
There was a problem hiding this comment.
P2: GET /api/catalogs/measurements can report an empty catalog/zero valuation when any measurement page or chunk fails. Since this helper returns [] on error, the handler cannot distinguish a real no-data result from a failed exhaustive read; throwing here would let the existing handler return its 500 path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/song_measurements/selectAllSongMeasurements.ts, line 48:
<comment>GET /api/catalogs/measurements can report an empty catalog/zero valuation when any measurement page or chunk fails. Since this helper returns [] on `error`, the handler cannot distinguish a real no-data result from a failed exhaustive read; throwing here would let the existing handler return its 500 path.</comment>
<file context>
@@ -0,0 +1,58 @@
+
+ if (error) {
+ console.error("Error fetching song_measurements:", error);
+ return [];
+ }
+
</file context>
| expect(builders[0].in).toHaveBeenCalledWith("song", ["ISRC1", "ISRC2"]); | ||
| expect(builders[0].eq).toHaveBeenCalledWith("platform", "spotify"); | ||
| expect(builders[0].eq).toHaveBeenCalledWith("metric", "platform_displayed_play_count"); | ||
| expect(builders[0].order).toHaveBeenCalledWith("captured_at", { ascending: false }); |
There was a problem hiding this comment.
P2: Missing assertion for the secondary sort column id. The implementation uses .order("id", { ascending: false }) to ensure deterministic pagination, but the test for the small-batch path only asserts .order("captured_at", ...) was called. If the id sort were accidentally removed, pagination could produce unstable pages (rows shifting between pages, silently duplicated or dropped during pagination).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/song_measurements/__tests__/selectAllSongMeasurements.test.ts, line 57:
<comment>Missing assertion for the secondary sort column `id`. The implementation uses `.order("id", { ascending: false })` to ensure deterministic pagination, but the test for the small-batch path only asserts `.order("captured_at", ...)` was called. If the `id` sort were accidentally removed, pagination could produce unstable pages (rows shifting between pages, silently duplicated or dropped during pagination).</comment>
<file context>
@@ -0,0 +1,102 @@
+ expect(builders[0].in).toHaveBeenCalledWith("song", ["ISRC1", "ISRC2"]);
+ expect(builders[0].eq).toHaveBeenCalledWith("platform", "spotify");
+ expect(builders[0].eq).toHaveBeenCalledWith("metric", "platform_displayed_play_count");
+ expect(builders[0].order).toHaveBeenCalledWith("captured_at", { ascending: false });
+ expect(builders[0].range).toHaveBeenCalledWith(0, 999);
+ expect(result).toEqual([row("ISRC1", 200), row("ISRC2", 50)]);
</file context>
| } | ||
|
|
||
| const songs = await selectCatalogSongTitles(catalogId); | ||
| const songs = await resolveCatalogSongsInScope({ catalogId, artistAccountId }); |
There was a problem hiding this comment.
P2: Artist-scoped valuation can be skewed because the stream total is filtered to the artist while catalog_age_years still comes from the whole catalog. Consider resolving the earliest release date from the scoped song set (or a scoped album/source lookup) when artist_account_id is provided.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/catalog/getCatalogMeasurementsHandler.ts, line 48:
<comment>Artist-scoped valuation can be skewed because the stream total is filtered to the artist while `catalog_age_years` still comes from the whole catalog. Consider resolving the earliest release date from the scoped song set (or a scoped album/source lookup) when `artist_account_id` is provided.</comment>
<file context>
@@ -34,18 +38,18 @@ export async function getCatalogMeasurementsHandler(request: NextRequest): Promi
}
- const songs = await selectCatalogSongTitles(catalogId);
+ const songs = await resolveCatalogSongsInScope({ catalogId, artistAccountId });
const titles = new Map(songs.map(s => [s.isrc, s.title]));
const [rows, earliestReleaseDate] = await Promise.all([
</file context>
| mockPages([{ data: null, error: { message: "boom" } }]); | ||
|
|
||
| expect(await selectAllSongMeasurements({ songs: ["ISRC1"], ...params })).toEqual([]); | ||
| consoleError.mockRestore(); |
There was a problem hiding this comment.
P3: The error test doesn't verify console.error was called. The test mocks console.error and asserts the return value, but never asserts the log was actually emitted. If a future refactor accidentally drops the console.error call, this test won't catch it. Add expect(consoleError).toHaveBeenCalledWith("Error fetching song_measurements:", { message: "boom" }) before the restore.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/song_measurements/__tests__/selectAllSongMeasurements.test.ts, line 100:
<comment>The error test doesn't verify `console.error` was called. The test mocks `console.error` and asserts the return value, but never asserts the log was actually emitted. If a future refactor accidentally drops the `console.error` call, this test won't catch it. Add `expect(consoleError).toHaveBeenCalledWith("Error fetching song_measurements:", { message: "boom" })` before the restore.</comment>
<file context>
@@ -0,0 +1,102 @@
+ mockPages([{ data: null, error: { message: "boom" } }]);
+
+ expect(await selectAllSongMeasurements({ songs: ["ISRC1"], ...params })).toEqual([]);
+ consoleError.mockRestore();
+ });
+});
</file context>
Preview verification — full authed pass ✅Preview deployment for Ground truth computed with read-only SQL against the shared prod DB (latest capture per ISRC over Authed checks (bearer for the owning test account)
Unauthed checks
Nothing pending: the test-account bearer authenticated against the preview (prod rejected it — previews use a different key salt — so the earlier prod 401 was salt, not expiry), so the full authed Done-when pass ran. 🤖 Generated with Claude Code |
1 participant
Implements the data plumbing v2: artist-scoped valuation api item of recoupable/chat#1850 — the contract is recoupable/docs#267 (merge that first).
What changed (same read path as api#757, two behaviors)
1. Fix: the silent 1,000-row cap (P1)
The endpoint computed
total_streams+ the valuation band over at most 1,000 rows — the Supabase default limit — with no error. Live repro on a 2,679-song catalog: exactly 1,000 measurements / 4.83B streams / $39.3M mid vs SQL ground truth 2,679 / 16.31B / ≈$132.5M (3.4× undervalued): #757 (comment)Now every read on the path paginates to exhaustion before dedupe/derivation:
lib/supabase/catalog_songs/selectCatalogSongTitles.ts—.range()loop (the tracklist itself capped at 1,000)lib/supabase/song_measurements/selectAllSongMeasurements.ts(new) — ISRC list chunked at 500 per query (boundedINclause / URL length) and each chunk.range()-paginated to exhaustion, with a stable secondary order for deterministic pageslib/supabase/song_artists/selectSongArtistIsrcs.ts(new) —.range()loop2. Feature: optional
artist_account_idquery param400on malformed) invalidateGetCatalogMeasurementsQuerysong_artists(catalog_songs ∩ song_artists, computed in the newlib/catalog/resolveCatalogSongsInScope.ts); absent → whole catalog (unchanged behavior)artist_account_idfield (uuid when scoped,nullwhen whole-catalog) so clients can verify the response scope before rendering artist-labeled numbers — pre-v2 deployments ignore the unknown param, and without the echo a client would show whole-catalog money under an artist label (the trust bug chat#1850 exists to kill)Verification
pnpm exec tsc --noEmit: 200 errors, all pre-existing in test files (0 new; 0 in touched files)pnpm lintclean[TEST] Full Roster Catalog (aggregate)fixture posted as a PR comment once the preview deploy is ReadyMerge order
recoupable/docs#267 (contract) → this PR → chat#1852 rework (hero consumes the filter + echo). Refs recoupable/chat#1850.
🤖 Generated with Claude Code
Summary by cubic
Adds an optional artist scope to GET /api/catalogs/measurements and removes the silent 1,000‑row cap by paginating all reads to exhaustion. Totals and valuation now cover all measured songs, and the response echoes the applied scope.
New Features
artist_account_id(UUID; 400 on malformed) to scope results tocatalog_songs ∩ song_artists.artist_account_idin the response (UUID when scoped,nullotherwise) so clients can verify the scope.Bug Fixes
catalog_songs,song_artists, andsong_measurementsto exhaustion; chunksong_measurementsINlists at 500 and use stable ordering for deterministic pages.Written for commit 637d8b8. Summary will update on new commits.