feat: persist catalog valuations + GET /api/catalogs/{catalogId}/valuations - #795
feat: persist catalog valuations + GET /api/catalogs/{catalogId}/valuations#795sweetmantech wants to merge 1 commit into
Conversation
|
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.
|
📝 WalkthroughWalkthroughAdds catalog valuation-history storage, best-effort persistence from valuation workflows, and an authenticated API route supporting latest-first queries, limits, access checks, and CORS preflight handling. ChangesCatalog Valuation History
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ValuationsRoute
participant validateGetCatalogValuationsQuery
participant selectAccountCatalog
participant selectCatalogValuations
Client->>ValuationsRoute: GET catalog valuations
ValuationsRoute->>validateGetCatalogValuationsQuery: Validate auth, catalogId, and limit
validateGetCatalogValuationsQuery-->>ValuationsRoute: Validated accountId and query
ValuationsRoute->>selectAccountCatalog: Check catalog-account link
selectAccountCatalog-->>ValuationsRoute: Catalog access result
ValuationsRoute->>selectCatalogValuations: Fetch latest-first valuation rows
selectCatalogValuations-->>ValuationsRoute: Limited valuation history
ValuationsRoute-->>Client: Success response with valuations
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
app/api/catalogs/[catalogId]/valuations/route.ts (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffInject the request-path dependencies.
getCatalogValuationsHandlerhard-wires validation and Supabase helpers, making this API flow harder to substitute in tests. Compose a handler with injected dependencies at the route boundary.As per path instructions, “Use dependency injection for services.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/catalogs/`[catalogId]/valuations/route.ts at line 3, Update the route boundary around getCatalogValuationsHandler to compose the handler with injected validation and Supabase dependencies instead of importing or relying on hard-wired helpers. Preserve the existing request and response behavior while passing the route-specific service implementations into the handler.Source: Path instructions
lib/supabase/catalog_valuations/selectCatalogValuations.ts (1)
12-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this helper under 20 lines.
Extract the query-error logging/handling into a small private helper; this function is 21 lines.
As per coding guidelines, “Flag functions longer than 20 lines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/supabase/catalog_valuations/selectCatalogValuations.ts` around lines 12 - 32, Reduce selectCatalogValuations below 20 lines by extracting the query-error logging and null-return handling into a small private helper. Keep the existing query and successful return behavior unchanged, and invoke the helper from the error branch.Source: Coding guidelines
lib/catalog/validateGetCatalogValuationsQuery.ts (2)
10-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the valuation-history limit contract.
Move the default, minimum, and maximum into a named configuration object so validation and future consumers cannot drift.
As per coding guidelines, “Use configuration objects instead of hardcoded values.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/catalog/validateGetCatalogValuationsQuery.ts` around lines 10 - 21, In the limit schema within validateGetCatalogValuationsQuery, replace the hardcoded default, minimum, and maximum values with a named configuration object. Define and reuse that object for the default, min, and max validation settings so the valuation-history limit contract is centralized for future consumers.Source: Coding guidelines
39-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit this validator into smaller focused helpers.
This function is 27 lines. Extract query parsing/error-response construction while preserving auth-first behavior.
As per coding guidelines, “Flag functions longer than 20 lines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/catalog/validateGetCatalogValuationsQuery.ts` around lines 39 - 65, Split validateGetCatalogValuationsQuery into focused helpers: extract query parameter parsing and validation, plus construction of the invalid-query NextResponse, while keeping validateAuthContext as the first operation and preserving the existing success and error response behavior. Keep validateGetCatalogValuationsQuery as the orchestration entry point and reuse the existing schema and CORS symbols.Source: Coding guidelines
lib/catalog/getCatalogValuationsHandler.ts (1)
23-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the handler below 20 lines.
Extract response mapping and the validated fetch/access flow into private helpers; the handler is currently 36 lines.
As per coding guidelines, “Flag functions longer than 20 lines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/catalog/getCatalogValuationsHandler.ts` around lines 23 - 58, Refactor getCatalogValuationsHandler to stay under 20 lines by extracting the row-to-response valuation mapping and the validated catalog access/fetch flow into private helper functions. Keep validation, authorization, error responses, success payload shape, and catch behavior unchanged; have the handler delegate to these helpers.Source: Coding guidelines
lib/catalog/getCatalogMeasurementsHandler.ts (1)
61-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftKeep the measurement handler focused and within the path limit.
getCatalogMeasurementsHandlerspans Lines 31-92 and now combines validation, access checks, three reads, valuation, persistence, and response shaping. Extract the new whole-catalog persistence decision into a focused use-case/helper so the handler remains a small request/response orchestrator.As per coding guidelines, functions longer than 20 lines must be flagged and functions should remain small and focused. As per path instructions, domain functions must remain under 50 lines and follow single responsibility.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/catalog/getCatalogMeasurementsHandler.ts` around lines 61 - 72, Extract the whole-catalog persistence decision from getCatalogMeasurementsHandler into a focused use-case/helper that handles the !artistAccountId condition and persistCatalogValuation call, then invoke it from the handler after valuation. Keep validation, reads, valuation, and response shaping in the handler while preserving daily deduplication and artist-scoped no-write behavior.Sources: Coding guidelines, Path instructions
lib/catalog/persistCatalogValuation.ts (1)
20-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftKeep the changed functions small and focused.
lib/catalog/persistCatalogValuation.ts#L20-L52: separate dedupe/atomic-write policy from error handling and the public persistence wrapper.lib/valuation/runValuationHandler.ts#L146-L154: move valuation-history persistence out of the 149-line request orchestrator.lib/catalog/getCatalogMeasurementsHandler.ts#L61-L72: move the whole-catalog persistence decision out of the 62-line measurements handler.As per coding guidelines, functions longer than 20 lines must be flagged and functions should remain small and focused. As per path instructions,
lib/**/*.tsdomain functions must remain under 50 lines and follow single responsibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/catalog/persistCatalogValuation.ts` around lines 20 - 52, Keep the persistence functions small and single-purpose: in lib/catalog/persistCatalogValuation.ts lines 20-52, extract deduplication and atomic-write logic from the public error-handling wrapper while preserving behavior; in lib/valuation/runValuationHandler.ts lines 146-154, move valuation-history persistence out of the request orchestrator into a focused helper; in lib/catalog/getCatalogMeasurementsHandler.ts lines 61-72, move the whole-catalog persistence decision into a focused helper. Ensure each affected domain function remains under 50 lines and ideally under 20 lines.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/catalog/persistCatalogValuation.ts`:
- Around line 34-48: Make the deduplication in persistCatalogValuation atomic
rather than relying on the selectCatalogValuations check followed by
insertCatalogValuation. Enforce one valuation per catalog and UTC day in the
database/storage layer using a unique UTC-day key or atomic insert/upsert,
allowing duplicate conflicts to remain harmless under the existing best-effort
behavior.
---
Nitpick comments:
In `@app/api/catalogs/`[catalogId]/valuations/route.ts:
- Line 3: Update the route boundary around getCatalogValuationsHandler to
compose the handler with injected validation and Supabase dependencies instead
of importing or relying on hard-wired helpers. Preserve the existing request and
response behavior while passing the route-specific service implementations into
the handler.
In `@lib/catalog/getCatalogMeasurementsHandler.ts`:
- Around line 61-72: Extract the whole-catalog persistence decision from
getCatalogMeasurementsHandler into a focused use-case/helper that handles the
!artistAccountId condition and persistCatalogValuation call, then invoke it from
the handler after valuation. Keep validation, reads, valuation, and response
shaping in the handler while preserving daily deduplication and artist-scoped
no-write behavior.
In `@lib/catalog/getCatalogValuationsHandler.ts`:
- Around line 23-58: Refactor getCatalogValuationsHandler to stay under 20 lines
by extracting the row-to-response valuation mapping and the validated catalog
access/fetch flow into private helper functions. Keep validation, authorization,
error responses, success payload shape, and catch behavior unchanged; have the
handler delegate to these helpers.
In `@lib/catalog/persistCatalogValuation.ts`:
- Around line 20-52: Keep the persistence functions small and single-purpose: in
lib/catalog/persistCatalogValuation.ts lines 20-52, extract deduplication and
atomic-write logic from the public error-handling wrapper while preserving
behavior; in lib/valuation/runValuationHandler.ts lines 146-154, move
valuation-history persistence out of the request orchestrator into a focused
helper; in lib/catalog/getCatalogMeasurementsHandler.ts lines 61-72, move the
whole-catalog persistence decision into a focused helper. Ensure each affected
domain function remains under 50 lines and ideally under 20 lines.
In `@lib/catalog/validateGetCatalogValuationsQuery.ts`:
- Around line 10-21: In the limit schema within
validateGetCatalogValuationsQuery, replace the hardcoded default, minimum, and
maximum values with a named configuration object. Define and reuse that object
for the default, min, and max validation settings so the valuation-history limit
contract is centralized for future consumers.
- Around line 39-65: Split validateGetCatalogValuationsQuery into focused
helpers: extract query parameter parsing and validation, plus construction of
the invalid-query NextResponse, while keeping validateAuthContext as the first
operation and preserving the existing success and error response behavior. Keep
validateGetCatalogValuationsQuery as the orchestration entry point and reuse the
existing schema and CORS symbols.
In `@lib/supabase/catalog_valuations/selectCatalogValuations.ts`:
- Around line 12-32: Reduce selectCatalogValuations below 20 lines by extracting
the query-error logging and null-return handling into a small private helper.
Keep the existing query and successful return behavior unchanged, and invoke the
helper from the error branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c1e9839e-f33b-4d37-ad4c-1e53caae05de
⛔ Files ignored due to path filters (7)
lib/catalog/__tests__/getCatalogMeasurementsHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/catalog/__tests__/getCatalogValuationsHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/catalog/__tests__/persistCatalogValuation.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/catalog/__tests__/validateGetCatalogValuationsQuery.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/supabase/catalog_valuations/__tests__/insertCatalogValuation.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/supabase/catalog_valuations/__tests__/selectCatalogValuations.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**types/database.types.tsis excluded by none and included by none
📒 Files selected for processing (8)
app/api/catalogs/[catalogId]/valuations/route.tslib/catalog/getCatalogMeasurementsHandler.tslib/catalog/getCatalogValuationsHandler.tslib/catalog/persistCatalogValuation.tslib/catalog/validateGetCatalogValuationsQuery.tslib/supabase/catalog_valuations/insertCatalogValuation.tslib/supabase/catalog_valuations/selectCatalogValuations.tslib/valuation/runValuationHandler.ts
| if (dedupeDaily) { | ||
| const latest = await selectCatalogValuations({ catalogId, limit: 1 }); | ||
| const newestDay = latest?.[0]?.measured_at?.slice(0, 10); | ||
| const today = new Date().toISOString().slice(0, 10); | ||
| if (newestDay === today) return; | ||
| } | ||
|
|
||
| await insertCatalogValuation({ | ||
| catalog_id: catalogId, | ||
| low: valuation.low, | ||
| mid: valuation.mid, | ||
| high: valuation.high, | ||
| measured_song_count: measuredSongCount, | ||
| total_streams: totalStreams, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make daily deduplication atomic.
Line 35 and Line 41 form a check-then-insert sequence. Concurrent whole-catalog requests can both observe no row for today and both insert, violating the at-most-one-per-UTC-day objective. Enforce this invariant in the database/storage layer with a unique UTC-day key or an atomic insert/upsert; duplicate conflicts can remain harmless under the best-effort policy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/catalog/persistCatalogValuation.ts` around lines 34 - 48, Make the
deduplication in persistCatalogValuation atomic rather than relying on the
selectCatalogValuations check followed by insertCatalogValuation. Enforce one
valuation per catalog and UTC day in the database/storage layer using a unique
UTC-day key or atomic insert/upsert, allowing duplicate conflicts to remain
harmless under the existing best-effort behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 15 files
Confidence score: 2/5
lib/catalog/getCatalogMeasurementsHandler.tscallspersistCatalogValuationfrom a GET path, so simple reads can mutatecatalog_valuationsand create unexpected side effects for retries/caching semantics — move persistence off the read endpoint (or make the write path explicitly idempotent and intentional).lib/catalog/persistCatalogValuation.tscan insert duplicate same-day history rows under concurrent reads, andlib/valuation/runValuationHandler.tscan persist zero-band valuations when aggregates fail, which risks polluting valuation history and downstream reporting — add a DB-enforced daily uniqueness/upsert and short-circuit persistence on aggregate query errors.lib/catalog/getCatalogValuationsHandler.tsmaps ownership lookup failures to 404, so Supabase/query outages can look like permanent “not found” responses and suppress client retries — keep an explicit error branch that returns 500 for backend failures.lib/catalog/__tests__/getCatalogValuationsHandler.test.ts,lib/catalog/__tests__/validateGetCatalogValuationsQuery.test.ts, and duplicated query-validation logic inlib/catalog/validateGetCatalogValuationsQuery.tsleave error-envelope and boundary behavior prone to drift — add the missing 500 body-safety and lower-bound tests, then consolidate shared validation/auth handling.
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/catalog/persistCatalogValuation.ts">
<violation number="1" location="lib/catalog/persistCatalogValuation.ts:41">
P2: Concurrent whole-catalog reads can create multiple daily history rows: each request can pass the lookup before either insert commits. Preserve the `dedupeDaily` guarantee with an atomic database-side operation/constraint rather than this read-then-insert check.</violation>
</file>
<file name="lib/catalog/__tests__/validateGetCatalogValuationsQuery.test.ts">
<violation number="1" location="lib/catalog/__tests__/validateGetCatalogValuationsQuery.test.ts:57">
P3: Missing test coverage for the lower limit boundary. The schema rejects limit=0 and limit=-1 with 400, but only the upper boundary (limit=101) and non-numeric input are tested. Add a test for limit=0 or limit=-1 to match the boundary coverage on the lower end.</violation>
</file>
<file name="lib/catalog/validateGetCatalogValuationsQuery.ts">
<violation number="1" location="lib/catalog/validateGetCatalogValuationsQuery.ts:43">
P3: Catalog query validation flow is now maintained in two near-identical implementations, so auth/error-envelope changes can drift between measurements and valuations. Consider extracting the shared auth/parse/error handling while keeping each endpoint schema local.</violation>
</file>
<file name="lib/catalog/getCatalogValuationsHandler.ts">
<violation number="1" location="lib/catalog/getCatalogValuationsHandler.ts:35">
P2: A failed ownership lookup is reported as “Catalog not found,” so a Supabase outage/query failure produces a permanent-looking 404 instead of a retryable 500. Preserve an error result distinct from an absent account-catalog link, then return 500 for that case.</violation>
</file>
<file name="lib/catalog/__tests__/getCatalogValuationsHandler.test.ts">
<violation number="1" location="lib/catalog/__tests__/getCatalogValuationsHandler.test.ts:113">
P2: The 500-error test only checks `response.status === 500` and never verifies the response body. Per the team's established convention, tests for 500 responses should assert that raw exception text does not leak into the body (e.g., assert `body.error === "Internal server error"` or that stack traces/DB errors are absent). Without this check, a future change that accidentally returns a dynamic error message would not be caught.</violation>
</file>
<file name="lib/valuation/runValuationHandler.ts">
<violation number="1" location="lib/valuation/runValuationHandler.ts:152">
P2: When `selectCatalogMeasurementsAggregate` returns null (DB error), the persist call silently writes a zero-band valuation (`{low:0, mid:0, high:0}`) with `measuredSongCount: 0, totalStreams: 0` into the valuation history, creating a misleading entry that looks like a legitimate measurement. The sibling handler `getCatalogMeasurementsHandler` guards against null aggregate with an early 500 return, and even the same file's `captureValuationLead` call uses `?? undefined` (meaning "unknown") rather than `?? 0` (which conflates "unknown" with "known zero"). Since the design intent is best-effort, consider skipping the persist when the aggregate query itself failed — that avoids polluting the history with a false zero-band row without adding a hard failure path.</violation>
</file>
<file name="lib/catalog/getCatalogMeasurementsHandler.ts">
<violation number="1" location="lib/catalog/getCatalogMeasurementsHandler.ts:64">
P1: Custom agent: **API Design Consistency and Maintainability**
This GET endpoint performs a database write by calling `persistCatalogValuation`, which inserts a row into `catalog_valuations`. The custom API-consistency rule requires GET for retrieval and POST for mutations or actions. Even though the write is idempotent (daily-deduped), it is still a side-effect mutation on a read endpoint, which breaks safe-method semantics and complicates caching. Consider moving the call to the existing POST valuation endpoint or exposing a dedicated POST route for persisting a valuation so that reads remain side-effect free.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant NextAPI as GET valuations Route
participant AuthV as Auth+Validator
participant CatDB as account_catalogs (Supabase)
participant ValDB as catalog_valuations (Supabase)
participant Persist as persistCatalogValuation
participant Insert as insertCatalogValuation (Supabase)
participant Existing as runValuationHandler / getCatalogMeasurementsHandler
Note over Client,Insert: NEW FLOW: GET /api/catalogs/{catalogId}/valuations?limit=N
Client->>NextAPI: GET /api/catalogs/{catalogId}/valuations?limit=30
NextAPI->>AuthV: validateGetCatalogValuationsQuery(request, catalogId)
AuthV->>AuthV: validateAuthContext (bearer or x-api-key)
alt Auth failure
AuthV-->>NextAPI: NextResponse 401
NextAPI-->>Client: 401
else Auth success → {accountId}
AuthV->>AuthV: parse & validate catalogId (UUID), limit (1-100, default 30)
alt Invalid params
AuthV-->>NextAPI: NextResponse 400
NextAPI-->>Client: 400
else Valid → {accountId, catalogId, limit}
AuthV-->>NextAPI: validated data
NextAPI->>CatDB: selectAccountCatalog({accountId, catalogId})
alt Catalog not owned or missing → null
CatDB-->>NextAPI: null
NextAPI-->>Client: 404 { status: "error", error: "Catalog not found" }
else Owned → link record
CatDB-->>NextAPI: link
NextAPI->>ValDB: selectCatalogValuations({catalogId, limit})
alt DB error → null
ValDB-->>NextAPI: null
NextAPI-->>Client: 500
else Success → rows []
ValDB-->>NextAPI: rows (latest-first)
NextAPI-->>Client: 200 { valuations: [{ low, mid, high, measured_song_count, total_streams, measured_at }] }
end
end
end
end
Note over Existing,Insert: NEW PERSIST HOOKS (inside existing flows)
Existing->>Existing: computeValuationBand() [pre-existing]
Existing-->>Existing: {low, mid, high, measuredSongCount, totalStreams}
Existing->>Persist: persistCatalogValuation({catalogId, valuation, measuredSongCount, totalStreams, dedupeDaily?})
alt dedupeDaily=true (whole-catalog read path)
Persist->>ValDB: selectCatalogValuations({catalogId, limit:1})
ValDB-->>Persist: latest row (or empty/null)
alt Latest row measured_at is today
Persist-->>Existing: skip (no insert)
else No row or earlier day
Persist->>Insert: insertCatalogValuation({catalog_id, low, mid, high, measured_song_count, total_streams})
Insert-->>Persist: inserted row or null
Note over Persist: Best-effort: errors logged, never thrown
Persist-->>Existing: void
end
else dedupeDaily=false (valuation run path)
Persist->>Insert: insertCatalogValuation(...)
Insert-->>Persist: inserted row or null
Persist-->>Existing: void
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Persist the whole-catalog band into the valuation history (chat#1889 | ||
| // row 15) — daily-deduped so page refreshes don't mint rows. Artist-scoped | ||
| // reads value a slice of the catalog, not the catalog, so they never write. | ||
| if (!artistAccountId) { |
There was a problem hiding this comment.
P1: Custom agent: API Design Consistency and Maintainability
This GET endpoint performs a database write by calling persistCatalogValuation, which inserts a row into catalog_valuations. The custom API-consistency rule requires GET for retrieval and POST for mutations or actions. Even though the write is idempotent (daily-deduped), it is still a side-effect mutation on a read endpoint, which breaks safe-method semantics and complicates caching. Consider moving the call to the existing POST valuation endpoint or exposing a dedicated POST route for persisting a valuation so that reads remain side-effect free.
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 64:
<comment>This GET endpoint performs a database write by calling `persistCatalogValuation`, which inserts a row into `catalog_valuations`. The custom API-consistency rule requires GET for retrieval and POST for mutations or actions. Even though the write is idempotent (daily-deduped), it is still a side-effect mutation on a read endpoint, which breaks safe-method semantics and complicates caching. Consider moving the call to the existing POST valuation endpoint or exposing a dedicated POST route for persisting a valuation so that reads remain side-effect free.</comment>
<file context>
@@ -57,6 +58,19 @@ export async function getCatalogMeasurementsHandler(
+ // Persist the whole-catalog band into the valuation history (chat#1889
+ // row 15) — daily-deduped so page refreshes don't mint rows. Artist-scoped
+ // reads value a slice of the catalog, not the catalog, so they never write.
+ if (!artistAccountId) {
+ await persistCatalogValuation({
+ catalogId,
</file context>
| if (newestDay === today) return; | ||
| } | ||
|
|
||
| await insertCatalogValuation({ |
There was a problem hiding this comment.
P2: Concurrent whole-catalog reads can create multiple daily history rows: each request can pass the lookup before either insert commits. Preserve the dedupeDaily guarantee with an atomic database-side operation/constraint rather than this read-then-insert check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/catalog/persistCatalogValuation.ts, line 41:
<comment>Concurrent whole-catalog reads can create multiple daily history rows: each request can pass the lookup before either insert commits. Preserve the `dedupeDaily` guarantee with an atomic database-side operation/constraint rather than this read-then-insert check.</comment>
<file context>
@@ -0,0 +1,52 @@
+ if (newestDay === today) return;
+ }
+
+ await insertCatalogValuation({
+ catalog_id: catalogId,
+ low: valuation.low,
</file context>
| const { accountId, catalogId, limit } = validated; | ||
|
|
||
| const link = await selectAccountCatalog({ accountId, catalogId }); | ||
| if (!link) { |
There was a problem hiding this comment.
P2: A failed ownership lookup is reported as “Catalog not found,” so a Supabase outage/query failure produces a permanent-looking 404 instead of a retryable 500. Preserve an error result distinct from an absent account-catalog link, then return 500 for that case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/catalog/getCatalogValuationsHandler.ts, line 35:
<comment>A failed ownership lookup is reported as “Catalog not found,” so a Supabase outage/query failure produces a permanent-looking 404 instead of a retryable 500. Preserve an error result distinct from an absent account-catalog link, then return 500 for that case.</comment>
<file context>
@@ -0,0 +1,58 @@
+ const { accountId, catalogId, limit } = validated;
+
+ const link = await selectAccountCatalog({ accountId, catalogId });
+ if (!link) {
+ return errorResponse("Catalog not found", 404);
+ }
</file context>
|
|
||
| const response = await getCatalogValuationsHandler(makeRequest(), catalogId); | ||
|
|
||
| expect(response.status).toBe(500); |
There was a problem hiding this comment.
P2: The 500-error test only checks response.status === 500 and never verifies the response body. Per the team's established convention, tests for 500 responses should assert that raw exception text does not leak into the body (e.g., assert body.error === "Internal server error" or that stack traces/DB errors are absent). Without this check, a future change that accidentally returns a dynamic error message would not be caught.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/catalog/__tests__/getCatalogValuationsHandler.test.ts, line 113:
<comment>The 500-error test only checks `response.status === 500` and never verifies the response body. Per the team's established convention, tests for 500 responses should assert that raw exception text does not leak into the body (e.g., assert `body.error === "Internal server error"` or that stack traces/DB errors are absent). Without this check, a future change that accidentally returns a dynamic error message would not be caught.</comment>
<file context>
@@ -0,0 +1,115 @@
+
+ const response = await getCatalogValuationsHandler(makeRequest(), catalogId);
+
+ expect(response.status).toBe(500);
+ });
+});
</file context>
| await persistCatalogValuation({ | ||
| catalogId: catalog.id, | ||
| valuation, | ||
| measuredSongCount: aggregate?.measuredSongCount ?? 0, |
There was a problem hiding this comment.
P2: When selectCatalogMeasurementsAggregate returns null (DB error), the persist call silently writes a zero-band valuation ({low:0, mid:0, high:0}) with measuredSongCount: 0, totalStreams: 0 into the valuation history, creating a misleading entry that looks like a legitimate measurement. The sibling handler getCatalogMeasurementsHandler guards against null aggregate with an early 500 return, and even the same file's captureValuationLead call uses ?? undefined (meaning "unknown") rather than ?? 0 (which conflates "unknown" with "known zero"). Since the design intent is best-effort, consider skipping the persist when the aggregate query itself failed — that avoids polluting the history with a false zero-band row without adding a hard failure path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/valuation/runValuationHandler.ts, line 152:
<comment>When `selectCatalogMeasurementsAggregate` returns null (DB error), the persist call silently writes a zero-band valuation (`{low:0, mid:0, high:0}`) with `measuredSongCount: 0, totalStreams: 0` into the valuation history, creating a misleading entry that looks like a legitimate measurement. The sibling handler `getCatalogMeasurementsHandler` guards against null aggregate with an early 500 return, and even the same file's `captureValuationLead` call uses `?? undefined` (meaning "unknown") rather than `?? 0` (which conflates "unknown" with "known zero"). Since the design intent is best-effort, consider skipping the persist when the aggregate query itself failed — that avoids polluting the history with a false zero-band row without adding a hard failure path.</comment>
<file context>
@@ -142,6 +143,16 @@ export async function runValuationHandler(request: NextRequest): Promise<NextRes
+ await persistCatalogValuation({
+ catalogId: catalog.id,
+ valuation,
+ measuredSongCount: aggregate?.measuredSongCount ?? 0,
+ totalStreams: aggregate?.totalStreams ?? 0,
+ });
</file context>
| expect((result as NextResponse).status).toBe(400); | ||
| }); | ||
|
|
||
| it("rejects limit above 100 with 400", async () => { |
There was a problem hiding this comment.
P3: Missing test coverage for the lower limit boundary. The schema rejects limit=0 and limit=-1 with 400, but only the upper boundary (limit=101) and non-numeric input are tested. Add a test for limit=0 or limit=-1 to match the boundary coverage on the lower end.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/catalog/__tests__/validateGetCatalogValuationsQuery.test.ts, line 57:
<comment>Missing test coverage for the lower limit boundary. The schema rejects limit=0 and limit=-1 with 400, but only the upper boundary (limit=101) and non-numeric input are tested. Add a test for limit=0 or limit=-1 to match the boundary coverage on the lower end.</comment>
<file context>
@@ -0,0 +1,74 @@
+ expect((result as NextResponse).status).toBe(400);
+ });
+
+ it("rejects limit above 100 with 400", async () => {
+ okAuth();
+
</file context>
| request: NextRequest, | ||
| catalogId: string, | ||
| ): Promise<NextResponse | GetCatalogValuationsQuery> { | ||
| const authResult = await validateAuthContext(request); |
There was a problem hiding this comment.
P3: Catalog query validation flow is now maintained in two near-identical implementations, so auth/error-envelope changes can drift between measurements and valuations. Consider extracting the shared auth/parse/error handling while keeping each endpoint schema local.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/catalog/validateGetCatalogValuationsQuery.ts, line 43:
<comment>Catalog query validation flow is now maintained in two near-identical implementations, so auth/error-envelope changes can drift between measurements and valuations. Consider extracting the shared auth/parse/error handling while keeping each endpoint schema local.</comment>
<file context>
@@ -0,0 +1,65 @@
+ request: NextRequest,
+ catalogId: string,
+): Promise<NextResponse | GetCatalogValuationsQuery> {
+ const authResult = await validateAuthContext(request);
+ if (authResult instanceof NextResponse) return authResult;
+
</file context>
…ations Every valuation run and each first whole-catalog measurements read of the day now writes a row to catalog_valuations, and the new GET returns the series latest-first (limit=1 = current value). chat#1889 row 15. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6bbc7ca to
cc0511d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (5)
lib/supabase/catalog_valuations/selectCatalogValuations.ts (1)
12-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the inline input shape out of this helper.
This function spans 21 lines solely because its parameter type is inline. Extract it to keep the function below the 20-line limit.
Proposed refactor
+type SelectCatalogValuationsParams = { + catalogId: string; + limit: number; +}; + export async function selectCatalogValuations({ catalogId, limit, -}: { - catalogId: string; - limit: number; -}): Promise<Tables<"catalog_valuations">[] | null> { +}: SelectCatalogValuationsParams): Promise<Tables<"catalog_valuations">[] | null> {As per coding guidelines, “Flag functions longer than 20 lines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/supabase/catalog_valuations/selectCatalogValuations.ts` around lines 12 - 18, Extract the inline `{ catalogId: string; limit: number }` parameter shape from `selectCatalogValuations` into a named type or interface, then use that symbol in the function signature while preserving the existing parameters and return type.Source: Coding guidelines
lib/catalog/validateGetCatalogValuationsQuery.ts (1)
39-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit authentication orchestration from query parsing.
This 27-line validator combines authentication, URL extraction, schema parsing, and error rendering. Extract a private query-parsing helper while preserving the current auth-first behavior.
As per coding guidelines, “Flag functions longer than 20 lines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/catalog/validateGetCatalogValuationsQuery.ts` around lines 39 - 65, The validateGetCatalogValuationsQuery function currently combines authentication with query extraction, schema validation, and error rendering. Extract the URL/query parsing and validation logic into a private helper, while keeping validateGetCatalogValuationsQuery responsible for auth-first orchestration and returning the same responses and validated data.Source: Coding guidelines
lib/catalog/getCatalogValuationsHandler.ts (1)
23-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecompose this handler into focused private helpers.
The 36-line handler performs access lookup, storage retrieval, response mapping, and error translation. Extract the row mapper and catalog-valuation loader so each function stays below the required limit.
As per coding guidelines, “Flag functions longer than 20 lines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/catalog/getCatalogValuationsHandler.ts` around lines 23 - 58, Refactor getCatalogValuationsHandler so it remains under 20 lines by extracting focused private helpers: one helper to map a valuation row into the response shape and another to load catalog valuations after the selectAccountCatalog access check, including the existing not-found and storage-error responses. Preserve the handler’s validation, error translation, and response behavior while reusing the helpers for row mapping and retrieval.Source: Coding guidelines
lib/valuation/runValuationHandler.ts (1)
149-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNormalize
totalStreamsonce.
aggregate?.totalStreams ?? 0is now duplicated across valuation, persistence, and lead capture. Store it in a local constant so those paths cannot diverge.As per coding guidelines, “Use constants for repeated values.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/valuation/runValuationHandler.ts` around lines 149 - 154, Define a local constant in the valuation flow for the normalized total stream count using aggregate?.totalStreams ?? 0, then reuse it in the valuation, persistCatalogValuation, and lead-capture paths. Remove the duplicated inline expressions while preserving the existing zero fallback.Source: Coding guidelines
lib/catalog/persistCatalogValuation.ts (1)
20-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit the oversized functions into focused collaborators.
The modified functions exceed the repository’s function-size limits; adding persistence increases their responsibilities.
lib/catalog/persistCatalogValuation.ts#L20-L52: extract daily-deduplication lookup from insert/error-handling orchestration.lib/valuation/runValuationHandler.ts#L146-L155: extract valuation-history persistence orchestration from the request workflow.lib/catalog/getCatalogMeasurementsHandler.ts#L61-L72: extract history-write orchestration from measurement retrieval/response construction.As per coding guidelines, “Flag functions longer than 20 lines.” As per path instructions, “Keep functions under 50 lines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/catalog/persistCatalogValuation.ts` around lines 20 - 52, Split the oversized persistence responsibilities into focused collaborators: in lib/catalog/persistCatalogValuation.ts lines 20-52, extract the daily-deduplication lookup from persistCatalogValuation while keeping insertion and error handling orchestration intact; in lib/valuation/runValuationHandler.ts lines 146-155, extract valuation-history persistence from the request workflow; in lib/catalog/getCatalogMeasurementsHandler.ts lines 61-72, extract history-write orchestration from measurement retrieval and response construction. Keep each affected function under the repository’s size limits and preserve existing behavior.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/catalog/getCatalogValuationsHandler.ts`:
- Around line 23-58: Refactor getCatalogValuationsHandler so it remains under 20
lines by extracting focused private helpers: one helper to map a valuation row
into the response shape and another to load catalog valuations after the
selectAccountCatalog access check, including the existing not-found and
storage-error responses. Preserve the handler’s validation, error translation,
and response behavior while reusing the helpers for row mapping and retrieval.
In `@lib/catalog/persistCatalogValuation.ts`:
- Around line 20-52: Split the oversized persistence responsibilities into
focused collaborators: in lib/catalog/persistCatalogValuation.ts lines 20-52,
extract the daily-deduplication lookup from persistCatalogValuation while
keeping insertion and error handling orchestration intact; in
lib/valuation/runValuationHandler.ts lines 146-155, extract valuation-history
persistence from the request workflow; in
lib/catalog/getCatalogMeasurementsHandler.ts lines 61-72, extract history-write
orchestration from measurement retrieval and response construction. Keep each
affected function under the repository’s size limits and preserve existing
behavior.
In `@lib/catalog/validateGetCatalogValuationsQuery.ts`:
- Around line 39-65: The validateGetCatalogValuationsQuery function currently
combines authentication with query extraction, schema validation, and error
rendering. Extract the URL/query parsing and validation logic into a private
helper, while keeping validateGetCatalogValuationsQuery responsible for
auth-first orchestration and returning the same responses and validated data.
In `@lib/supabase/catalog_valuations/selectCatalogValuations.ts`:
- Around line 12-18: Extract the inline `{ catalogId: string; limit: number }`
parameter shape from `selectCatalogValuations` into a named type or interface,
then use that symbol in the function signature while preserving the existing
parameters and return type.
In `@lib/valuation/runValuationHandler.ts`:
- Around line 149-154: Define a local constant in the valuation flow for the
normalized total stream count using aggregate?.totalStreams ?? 0, then reuse it
in the valuation, persistCatalogValuation, and lead-capture paths. Remove the
duplicated inline expressions while preserving the existing zero fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e6baf6a0-1115-417e-9994-baee849c8efe
⛔ Files ignored due to path filters (7)
lib/catalog/__tests__/getCatalogMeasurementsHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/catalog/__tests__/getCatalogValuationsHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/catalog/__tests__/persistCatalogValuation.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/catalog/__tests__/validateGetCatalogValuationsQuery.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/supabase/catalog_valuations/__tests__/insertCatalogValuation.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/supabase/catalog_valuations/__tests__/selectCatalogValuations.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**types/database.types.tsis excluded by none and included by none
📒 Files selected for processing (8)
app/api/catalogs/[catalogId]/valuations/route.tslib/catalog/getCatalogMeasurementsHandler.tslib/catalog/getCatalogValuationsHandler.tslib/catalog/persistCatalogValuation.tslib/catalog/validateGetCatalogValuationsQuery.tslib/supabase/catalog_valuations/insertCatalogValuation.tslib/supabase/catalog_valuations/selectCatalogValuations.tslib/valuation/runValuationHandler.ts
Preview verification — 2026-07-30, all documented paths livePreview
Docs ↔ API ↔ live reconciliation: response field names, envelope ( Bonus: rows 8–12 accidentally demo the product story — the series reads Test debris: the agent account + its Del Water Gap catalog + 2 valuation rows remain (harmless; happy to clean up post-merge). Merge order: docs#279 ✅ → database#52 ✅ → this. |
1 participant
Summary
Row 15 of chat#1889: the keystone.
est_catalog_valuewas computed on every run and read but never persisted. This PR writes it to the newcatalog_valuationshistory table and exposes the series viaGET /api/catalogs/{catalogId}/valuations.Merge order: docs#279 (contract) → database#52 (table — hard dependency, must merge before this) → this PR.
What it does
Persist (two sites, one helper):
lib/catalog/persistCatalogValuation.ts— best-effort (never fails the caller), optionaldedupeDailyrunValuationHandler— every valuation run writes a row (each run is a fresh measurement)getCatalogMeasurementsHandler— whole-catalog reads write at most one row per catalog per UTC day; artist-scoped reads never write (they value a slice, not the catalog)Read:
GET /api/catalogs/{catalogId}/valuations?limit=— latest-first series, limit 1–100 default 30,limit=1= current value. Auth viavalidateAuthContext(bearer or x-api-key); unowned/missing catalog → 404; bad catalogId/limit → 400. Flat{ status, valuations }with rows{ low, mid, high, measured_song_count, total_streams, measured_at }, matching docs#279 exactly.Supabase layer:
lib/supabase/catalog_valuations/{insertCatalogValuation,selectCatalogValuations}.ts+ hand-addedcatalog_valuationsblock intypes/database.types.ts(regenerates identically once database#52 merges).Tests (TDD, red→green)
18 new tests across 5 new suites + 2 wiring cases added to
getCatalogMeasurementsHandler.test.ts(persists daily-deduped on whole-catalog reads; never on artist-scoped reads). All confirmed RED before implementation.tsc --noEmit: 236 errors — the pre-existing baseline, none in touched filesrunValuationHandlerhas no unit harness (repo convention for that orchestrator); its persist wiring will be verified live on the preview (run a valuation → row appears in the GET).Closes nothing on its own — tracked in chat#1889 row 15.
🤖 Generated with Claude Code
Summary by cubic
Persist catalog valuation bands to
catalog_valuationsand add GET /api/catalogs/{catalogId}/valuations to read the history (limit=1 returns the current value). This turns ephemeralest_catalog_valueinto a stored time series for charts and audits.New Features
/api/catalogs/{catalogId}/valuations?limit=(1–100, default 30), latest-first. Auth via bearer orx-api-key; 400 on invalid catalogId/limit; 404 if catalog is missing or not owned. Returns{ valuations: [{ low, mid, high, measured_song_count, total_streams, measured_at }] }.Migration
catalog_valuationstable is deployed before releasing this endpoint. No client changes required.Written for commit cc0511d. Summary will update on new commits.
Summary by CodeRabbit