feat(research): measurements read collection (chat#1796)#669
Conversation
The read half of the chat#1796 consolidation, against docs#242, TDD throughout.
- GET /api/research/tracks/{id}/measurements — a track's measured series from
the store (granularity=daily); aggregate=run_rate returns the trailing-window
annualized run-rate (a projection via computePlaycountDeltas). Provider-neutral
{id} (ISRC or Spotify track id, resolved to ISRC). Replaces track/historic-stats
+ track/playcount-deltas.
- GET /api/research/albums/{id}/measurements — latest count per track; a thin
remap over getAlbumPlaycounts. Replaces playcounts.
Heavy reuse of existing store reads (selectSongMeasurements, computePlaycountDeltas,
getAlbumPlaycounts); legacy endpoints keep serving (deprecated in docs#242 only).
24 new unit tests (resolver, both data fns, both validators, both handlers);
full lib/research suite green (268); tsc + lint clean on touched files.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 48 minutes and 22 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (9)
✨ 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 |
There was a problem hiding this comment.
10 issues found across 16 files
Confidence score: 3/5
- In
lib/research/measurements/resolveTrackIsrc.ts, ISRC-like IDs are not uppercased before lookup, so valid lowercase input can miss indexed records and return false 404s to users — normalize to uppercase at parse/resolve time before merging. lib/research/measurements/validateGetTrackMeasurementsRequest.tscurrently accepts invalidwindowvalues (including0d) by defaulting to 365 and has no max bound, which can yield misleading 200 responses or overflow-prone date math downstream — enforce strict 400s for invalid/zero values and add a sane upper limit.- In
lib/research/measurements/getTrackMeasurements.ts, the?? deltas[0]fallback is both unreachable today and unsafe if logic changes, because it could return a delta for the wrong platform/metric — remove the fallback and keep selection strictly keyed to the filtered row. - There are a few contract-safety gaps around album/handler paths:
lib/research/measurements/getAlbumMeasurements.tsuses a local casted row shape and duplicated metric constant that can drift silently, and related tests/handlers (__tests__/getAlbumMeasurementsHandler.test.ts,getTrackMeasurementsHandler.ts,getAlbumMeasurementsHandler.ts,__tests__/validateGetAlbumMeasurementsRequest.test.ts) don’t fully lock error/credit-gate behavior — centralize shared types/constants and tighten assertions before merge.
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/research/measurements/resolveTrackIsrc.ts">
<violation number="1" location="lib/research/measurements/resolveTrackIsrc.ts:15">
P1: ISRC-shaped ids pass through without uppercasing. ISO 3901 ISRCs are uppercase; the store indexes them that way, so a lowercase ISRC input causes a false 404 on the subsequent measurements query.</violation>
</file>
<file name="lib/research/measurements/getAlbumMeasurementsHandler.ts">
<violation number="1" location="lib/research/measurements/getAlbumMeasurementsHandler.ts:31">
P2: 500 response message is "Internal error" but should be "Internal server error" per team convention and feedback (b9a9c612).</violation>
</file>
<file name="lib/research/measurements/__tests__/getAlbumMeasurementsHandler.test.ts">
<violation number="1" location="lib/research/measurements/__tests__/getAlbumMeasurementsHandler.test.ts:24">
P2: 500 error test does not verify the response body is a safe hardcoded string ("Internal error") and does not assert exception details ("boom") are absent from the response.</violation>
</file>
<file name="lib/research/measurements/getTrackMeasurements.ts">
<violation number="1" location="lib/research/measurements/getTrackMeasurements.ts:63">
P2: The `?? deltas[0]` fallback is unreachable dead code (rows are pre-filtered by platform/metric, so find always matches) and semantically wrong if ever reached — it could return a delta for a different platform/metric than requested. Remove the fallback so a failed find yields `undefined` → `aggregate: null`.</violation>
</file>
<file name="lib/research/measurements/getTrackMeasurementsHandler.ts">
<violation number="1" location="lib/research/measurements/getTrackMeasurementsHandler.ts:32">
P2: 500 error response uses "Internal error" instead of the team-standard "Internal server error".</violation>
</file>
<file name="lib/research/measurements/validateGetTrackMeasurementsRequest.ts">
<violation number="1" location="lib/research/measurements/validateGetTrackMeasurementsRequest.ts:45">
P3: No upper bound on windowDays; extremely large values cause integer overflow in downstream date arithmetic (`windowDays * DAY_MS`).</violation>
<violation number="2" location="lib/research/measurements/validateGetTrackMeasurementsRequest.ts:45">
P2: Invalid `window` values silently default to 365 instead of 400-ing, and `window=0d` passes validation producing a meaningless null aggregate with 200 status.</violation>
</file>
<file name="lib/research/measurements/getAlbumMeasurements.ts">
<violation number="1" location="lib/research/measurements/getAlbumMeasurements.ts:3">
P2: Duplicate `METRIC` constant — same value already exists in `getAlbumPlaycounts.ts`. If the metric name changes upstream, this copy won't be updated. Violates DRY and AGENTS.md convention that shared constants belong in `lib/const.ts`.</violation>
<violation number="2" location="lib/research/measurements/getAlbumMeasurements.ts:31">
P2: Local `PlaycountRow` type + `as` cast must manually stay in sync with `getAlbumPlaycounts`'s output shape. If upstream renames or removes a field, the cast still compiles but `playcounts` becomes `undefined` at runtime, crashing `.map()`.</violation>
</file>
<file name="lib/research/measurements/__tests__/validateGetAlbumMeasurementsRequest.test.ts">
<violation number="1" location="lib/research/measurements/__tests__/validateGetAlbumMeasurementsRequest.test.ts:27">
P3: Add a 402 short-circuit case for `ensureResearchCredits`. A regression that removes or bypasses the album credit gate would still pass this test.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as External Client
participant Router as Next.js Router
participant Handler as Handler Layer
participant Validator as Validator Layer
participant Data as Data Layer
participant Auth as Auth Service
participant Credit as Credit Service
participant Resolver as ID Resolver
participant DB as Supabase DB
participant Legacy as Legacy Endpoints
Note over Client,Legacy: Track Measurements - GET /api/research/tracks/{id}/measurements
Client->>Router: GET /api/research/tracks/{id}/measurements?aggregate=run_rate&window=365d
Router->>Handler: getTrackMeasurementsHandler(request, id)
Handler->>Validator: validateGetTrackMeasurementsRequest(request, id)
Validator->>Auth: validateAuthContext(request)
alt Auth Fails (401)
Auth-->>Validator: NextResponse(401)
Validator-->>Handler: NextResponse(401)
Handler-->>Client: 401 Unauthorized
else Auth Succeeds
Auth-->>Validator: { accountId }
Validator->>Credit: ensureResearchCredits(accountId)
alt Insufficient Credits
Credit-->>Validator: NextResponse(403)
Validator-->>Handler: NextResponse(403)
Handler-->>Client: 403 Insufficient Credits
else Credits Valid
Credit-->>Validator: null
Validator->>Validator: Parse params (aggregate, window, platform, metric)
alt Invalid aggregate param
Validator-->>Handler: NextResponse(400)
Handler-->>Client: 400 Bad Request
else Params Valid
Validator-->>Handler: ValidatedGetTrackMeasurementsRequest
end
end
end
Handler->>Data: getTrackMeasurements(validated)
Data->>Resolver: resolveTrackIsrc(id)
alt id is ISRC-shaped
Resolver->>Resolver: Pattern match (no DB call)
Resolver-->>Data: isrc
else id is Spotify track id
Resolver->>DB: selectSongIdentifiers(spotify, track_id, values=[id])
DB-->>Resolver: [{ song: isrc }] or []
alt No mapping found
Resolver-->>Data: null
Data-->>Handler: { error: "Unknown track id", status: 404 }
Handler-->>Client: 404 Not Found
else Mapped
Resolver-->>Data: isrc
end
end
Data->>DB: selectSongMeasurements(song=isrc, platform, metric)
alt No measurements found
DB-->>Data: []
Data-->>Handler: { error: "No measurements...", status: 404 }
Handler-->>Client: 404 Not Found
else Measurements exist
DB-->>Data: rows[]
Data->>Credit: deductCredits(accountId)
Credit-->>Data: ok
alt Default series (no aggregate)
Data->>Data: Sort rows ascending by date
Data-->>Handler: { data: { series: [{date, value, data_source}] } }
Handler-->>Client: 200 JSON with series array
else Run-rate aggregate (aggregate=run_rate)
Data->>Data: Calculate window start date
Data->>Data: computePlaycountDeltas(rows, { since })
Data->>Data: Find matching delta for platform/metric
alt Sufficient history for delta
Data-->>Handler: { data: { aggregate: { kind: "run_rate", window_days, delta, run_rate_annualized } } }
else Insufficient history
Data-->>Handler: { data: { aggregate: null } }
end
Handler-->>Client: 200 JSON with aggregate object
end
end
Note over Client,Legacy: Album Measurements - GET /api/research/albums/{id}/measurements
Client->>Router: GET /api/research/albums/{id}/measurements
Router->>Handler: getAlbumMeasurementsHandler(request, id)
Handler->>Validator: validateGetAlbumMeasurementsRequest(request, id)
Validator->>Auth: validateAuthContext(request)
alt Auth Fails
Auth-->>Validator: NextResponse(401)
Validator-->>Handler: NextResponse(401)
Handler-->>Client: 401 Unauthorized
else Auth Succeeds
Auth-->>Validator: { accountId }
Validator->>Credit: ensureResearchCredits(accountId)
Credit-->>Validator: null or NextResponse
Validator-->>Handler: { accountId, spotifyAlbumId: id }
end
Handler->>Data: getAlbumMeasurements({ accountId, spotifyAlbumId })
Data->>Legacy: getAlbumPlaycounts({ accountId, spotifyAlbumId })
alt Underlying read fails
Legacy-->>Data: { error: "No snapshot", status: 404 }
Data-->>Handler: { error: "No snapshot", status: 404 }
Handler-->>Client: 404 Not Found
else Underlying read succeeds
Legacy-->>Data: { data: { playcounts: [...] } }
Data->>Data: Remap playcounts to measurements shape
Data-->>Handler: { data: { status: "success", id, platform, metric, measurements: [...] } }
Handler-->>Client: 200 JSON with measurements array
end
Note over Client,Legacy: Legacy endpoints remain available (deprecated)
Client->>Router: GET /api/research/track/historic-stats (deprecated)
Router->>Legacy: Legacy handler
Legacy-->>Client: Response
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| * @returns The ISRC, or null when a non-ISRC id maps to nothing | ||
| */ | ||
| export async function resolveTrackIsrc(id: string): Promise<string | null> { | ||
| if (ISRC_PATTERN.test(id)) return id; |
There was a problem hiding this comment.
P1: ISRC-shaped ids pass through without uppercasing. ISO 3901 ISRCs are uppercase; the store indexes them that way, so a lowercase ISRC input causes a false 404 on the subsequent measurements query.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/measurements/resolveTrackIsrc.ts, line 15:
<comment>ISRC-shaped ids pass through without uppercasing. ISO 3901 ISRCs are uppercase; the store indexes them that way, so a lowercase ISRC input causes a false 404 on the subsequent measurements query.</comment>
<file context>
@@ -0,0 +1,23 @@
+ * @returns The ISRC, or null when a non-ISRC id maps to nothing
+ */
+export async function resolveTrackIsrc(id: string): Promise<string | null> {
+ if (ISRC_PATTERN.test(id)) return id;
+
+ const rows = await selectSongIdentifiers({
</file context>
| return successResponse(result.data as Record<string, unknown>); | ||
| } catch (error) { | ||
| console.error("[ERROR] getAlbumMeasurementsHandler:", error); | ||
| return errorResponse("Internal error", 500); |
There was a problem hiding this comment.
P2: 500 response message is "Internal error" but should be "Internal server error" per team convention and feedback (b9a9c612).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/measurements/getAlbumMeasurementsHandler.ts, line 31:
<comment>500 response message is "Internal error" but should be "Internal server error" per team convention and feedback (b9a9c612).</comment>
<file context>
@@ -0,0 +1,33 @@
+ return successResponse(result.data as Record<string, unknown>);
+ } catch (error) {
+ console.error("[ERROR] getAlbumMeasurementsHandler:", error);
+ return errorResponse("Internal error", 500);
+ }
+}
</file context>
| vi.mocked(getAlbumMeasurements).mockResolvedValue({ | ||
| data: { status: "success", id: "AL1", measurements: [] }, | ||
| }); | ||
| const res = await getAlbumMeasurementsHandler(req(), "AL1"); |
There was a problem hiding this comment.
P2: 500 error test does not verify the response body is a safe hardcoded string ("Internal error") and does not assert exception details ("boom") are absent from the response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/measurements/__tests__/getAlbumMeasurementsHandler.test.ts, line 24:
<comment>500 error test does not verify the response body is a safe hardcoded string ("Internal error") and does not assert exception details ("boom") are absent from the response.</comment>
<file context>
@@ -0,0 +1,52 @@
+ vi.mocked(getAlbumMeasurements).mockResolvedValue({
+ data: { status: "success", id: "AL1", measurements: [] },
+ });
+ const res = await getAlbumMeasurementsHandler(req(), "AL1");
+ expect(res.status).toBe(200);
+ expect(await res.json()).toMatchObject({ id: "AL1" });
</file context>
| .slice(0, 10); | ||
| const deltas = computePlaycountDeltas(rows, { since }); | ||
| const match = | ||
| deltas.find(d => d.platform === params.platform && d.metric === params.metric) ?? deltas[0]; |
There was a problem hiding this comment.
P2: The ?? deltas[0] fallback is unreachable dead code (rows are pre-filtered by platform/metric, so find always matches) and semantically wrong if ever reached — it could return a delta for a different platform/metric than requested. Remove the fallback so a failed find yields undefined → aggregate: null.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/measurements/getTrackMeasurements.ts, line 63:
<comment>The `?? deltas[0]` fallback is unreachable dead code (rows are pre-filtered by platform/metric, so find always matches) and semantically wrong if ever reached — it could return a delta for a different platform/metric than requested. Remove the fallback so a failed find yields `undefined` → `aggregate: null`.</comment>
<file context>
@@ -0,0 +1,84 @@
+ .slice(0, 10);
+ const deltas = computePlaycountDeltas(rows, { since });
+ const match =
+ deltas.find(d => d.platform === params.platform && d.metric === params.metric) ?? deltas[0];
+ const aggregate = match
+ ? {
</file context>
| return successResponse(result.data as Record<string, unknown>); | ||
| } catch (error) { | ||
| console.error("[ERROR] getTrackMeasurementsHandler:", error); | ||
| return errorResponse("Internal error", 500); |
There was a problem hiding this comment.
P2: 500 error response uses "Internal error" instead of the team-standard "Internal server error".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/measurements/getTrackMeasurementsHandler.ts, line 32:
<comment>500 error response uses "Internal error" instead of the team-standard "Internal server error".</comment>
<file context>
@@ -0,0 +1,34 @@
+ return successResponse(result.data as Record<string, unknown>);
+ } catch (error) {
+ console.error("[ERROR] getTrackMeasurementsHandler:", error);
+ return errorResponse("Internal error", 500);
+ }
+}
</file context>
| if (short) return short; | ||
|
|
||
| const windowMatch = (searchParams.get("window") ?? "").match(/^(\d+)d?$/); | ||
| const windowDays = windowMatch ? Number(windowMatch[1]) : DEFAULT_WINDOW_DAYS; |
There was a problem hiding this comment.
P2: Invalid window values silently default to 365 instead of 400-ing, and window=0d passes validation producing a meaningless null aggregate with 200 status.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/measurements/validateGetTrackMeasurementsRequest.ts, line 45:
<comment>Invalid `window` values silently default to 365 instead of 400-ing, and `window=0d` passes validation producing a meaningless null aggregate with 200 status.</comment>
<file context>
@@ -0,0 +1,55 @@
+ if (short) return short;
+
+ const windowMatch = (searchParams.get("window") ?? "").match(/^(\d+)d?$/);
+ const windowDays = windowMatch ? Number(windowMatch[1]) : DEFAULT_WINDOW_DAYS;
+
+ return {
</file context>
| @@ -0,0 +1,50 @@ | |||
| import { getAlbumPlaycounts } from "@/lib/research/playcounts/getAlbumPlaycounts"; | |||
|
|
|||
| const METRIC = "platform_displayed_play_count"; | |||
There was a problem hiding this comment.
P2: Duplicate METRIC constant — same value already exists in getAlbumPlaycounts.ts. If the metric name changes upstream, this copy won't be updated. Violates DRY and AGENTS.md convention that shared constants belong in lib/const.ts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/measurements/getAlbumMeasurements.ts, line 3:
<comment>Duplicate `METRIC` constant — same value already exists in `getAlbumPlaycounts.ts`. If the metric name changes upstream, this copy won't be updated. Violates DRY and AGENTS.md convention that shared constants belong in `lib/const.ts`.</comment>
<file context>
@@ -0,0 +1,50 @@
+import { getAlbumPlaycounts } from "@/lib/research/playcounts/getAlbumPlaycounts";
+
+const METRIC = "platform_displayed_play_count";
+
+export type GetAlbumMeasurementsResult = { data: unknown } | { error: string; status: number };
</file context>
| const result = await getAlbumPlaycounts(params); | ||
| if ("error" in result) return result; | ||
|
|
||
| const { playcounts } = result.data as { playcounts: PlaycountRow[] }; |
There was a problem hiding this comment.
P2: Local PlaycountRow type + as cast must manually stay in sync with getAlbumPlaycounts's output shape. If upstream renames or removes a field, the cast still compiles but playcounts becomes undefined at runtime, crashing .map().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/measurements/getAlbumMeasurements.ts, line 31:
<comment>Local `PlaycountRow` type + `as` cast must manually stay in sync with `getAlbumPlaycounts`'s output shape. If upstream renames or removes a field, the cast still compiles but `playcounts` becomes `undefined` at runtime, crashing `.map()`.</comment>
<file context>
@@ -0,0 +1,50 @@
+ const result = await getAlbumPlaycounts(params);
+ if ("error" in result) return result;
+
+ const { playcounts } = result.data as { playcounts: PlaycountRow[] };
+ const measurements = playcounts.map(p => ({
+ isrc: p.isrc,
</file context>
| if (short) return short; | ||
|
|
||
| const windowMatch = (searchParams.get("window") ?? "").match(/^(\d+)d?$/); | ||
| const windowDays = windowMatch ? Number(windowMatch[1]) : DEFAULT_WINDOW_DAYS; |
There was a problem hiding this comment.
P3: No upper bound on windowDays; extremely large values cause integer overflow in downstream date arithmetic (windowDays * DAY_MS).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/measurements/validateGetTrackMeasurementsRequest.ts, line 45:
<comment>No upper bound on windowDays; extremely large values cause integer overflow in downstream date arithmetic (`windowDays * DAY_MS`).</comment>
<file context>
@@ -0,0 +1,55 @@
+ if (short) return short;
+
+ const windowMatch = (searchParams.get("window") ?? "").match(/^(\d+)d?$/);
+ const windowDays = windowMatch ? Number(windowMatch[1]) : DEFAULT_WINDOW_DAYS;
+
+ return {
</file context>
| expect((r as NextResponse).status).toBe(401); | ||
| }); | ||
|
|
||
| it("returns account + album id on success", async () => { |
There was a problem hiding this comment.
P3: Add a 402 short-circuit case for ensureResearchCredits. A regression that removes or bypasses the album credit gate would still pass this test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/research/measurements/__tests__/validateGetAlbumMeasurementsRequest.test.ts, line 27:
<comment>Add a 402 short-circuit case for `ensureResearchCredits`. A regression that removes or bypasses the album credit gate would still pass this test.</comment>
<file context>
@@ -0,0 +1,31 @@
+ expect((r as NextResponse).status).toBe(401);
+ });
+
+ it("returns account + album id on success", async () => {
+ const r = await validateGetAlbumMeasurementsRequest(req(), "AL1");
+ expect(r).toEqual({ accountId: "acc_1", spotifyAlbumId: "AL1" });
</file context>
Preview verification ✅ — Done-when criteria metTested the live preview (
Equivalence vs the (still-serving) legacy endpoints — identical
One observation (non-blocking)The series returned exactly 1,000 points (oldest = 2023-09-30) — PostgREST's default row cap on Verified by: @sweetmantech session. Auth via a preview-environment agent key (not echoed). |
The read half of the chat#1796 consolidation (the slice deferred from api#668), against the docs contract in recoupable/docs#242. TDD throughout — RED→GREEN per unit.
New reads — one
measurementscollectionGET /api/research/tracks/{id}/measurements— a track's measured series from the store (granularity=daily);aggregate=run_rate&window=365dreturns the trailing-window annualized run-rate (a projection of the same series viacomputePlaycountDeltas).{id}is provider-neutral (ISRC or Spotify track id → resolved to ISRC). Replacestrack/historic-stats+track/playcount-deltas.GET /api/research/albums/{id}/measurements— latest measured count per track; a thin remap overgetAlbumPlaycounts. Replacesplaycounts.Layered per house style (
route → handler → validate → data, one exported fn per file), reusing the existing store reads —selectSongMeasurements,computePlaycountDeltas,getAlbumPlaycounts. No vendor calls in the request path.Non-breaking
Legacy endpoints keep serving — they're marked
deprecatedin docs#242 only. Nothing is removed.Testing
lib/researchsuite: 268 passed, no regressions.tsc --noEmitclean on touched files; lint clean.Completes the #1796 api surface
With this + api#668 (the write resource + retryable drain), the full two-resource model from #1796 is implemented:
measurements(read) +measurement-jobs(write). Basetest; awaits review (not self-merged).🤖 Generated with Claude Code
Summary by cubic
Adds a unified
measurementsread collection for research data, implementing the read side of chat#1796. This consolidates track and album stats into two endpoints without breaking existing APIs.New Features
/api/research/tracks/{id}/measurements— daily series for a track; supportsaggregate=run_rate&window=365d(annualized run-rate).{id}accepts ISRC or Spotify track id (resolved to ISRC). Defaults:platform=spotify,metric=platform_displayed_play_count,window=365d./api/research/albums/{id}/measurements— latest measured count per track for an album (thin remap over store reads).Migration
aggregate=run_rateand optionalwindowlike90d(default365d).Written for commit 16fc4b1. Summary will update on new commits.