feat: catalog_valuations — persisted valuation history (chat#1889 row 15) - #52
Merged
Merged
Conversation
chat#1889 row 15 (keystone). One row per band computation; read is always latest-first per catalog, hence the (catalog_id, measured_at DESC) index. FK CASCADE to catalogs: a valuation row is meaningless without its catalog. Contract: docs#279. Co-Authored-By: Claude Opus 5 (1M context) <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. |
|
Updates to Preview Branch (feat/catalog-valuations-table) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
📝 WalkthroughWalkthroughA Supabase migration creates ChangesCatalog valuation history
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
13 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@supabase/migrations/20260729230000_create_catalog_valuations.sql`:
- Around line 21-31: Add table-level CHECK constraints to
public.catalog_valuations ensuring low, mid, and high are non-negative and
ordered low <= mid <= high, and measured_song_count and total_streams are
non-negative. Keep the existing columns and defaults unchanged.
- Around line 21-31: Update the catalog_valuations table definition to establish
its intended API access path, matching the neighboring catalog tables: either
grant the required privileges to the service-role endpoint or enable RLS and add
policies permitting authenticated reads for GET
/api/catalogs/{catalogId}/valuations. Ensure service-role writes remain
permitted and the catalog_id scope is enforced consistently with existing
policies.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f51c220b-270a-4da6-9cc3-78f1221a1057
📒 Files selected for processing (1)
supabase/migrations/20260729230000_create_catalog_valuations.sql
Comment on lines
+21
to
+31
| CREATE TABLE IF NOT EXISTS public.catalog_valuations ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| catalog_id UUID NOT NULL REFERENCES public.catalogs(id) ON DELETE CASCADE, | ||
| low NUMERIC NOT NULL, -- band low, USD | ||
| mid NUMERIC NOT NULL, -- band midpoint, USD | ||
| high NUMERIC NOT NULL, -- band high, USD | ||
| measured_song_count INTEGER NOT NULL, -- songs measured in the underlying capture | ||
| total_streams BIGINT NOT NULL, -- whole-catalog lifetime streams at measurement | ||
| measured_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), | ||
| created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now() | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce valuation invariants in the schema.
NOT NULL still permits negative counts/streams and invalid bands such as low > mid or mid > high. Add table-level CHECK constraints so every writer preserves the history contract.
Proposed fix
measured_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(),
- created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now()
+ created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(),
+ CONSTRAINT catalog_valuations_band_order_chk
+ CHECK (low >= 0 AND low <= mid AND mid <= high),
+ CONSTRAINT catalog_valuations_song_count_chk
+ CHECK (measured_song_count >= 0),
+ CONSTRAINT catalog_valuations_streams_chk
+ CHECK (total_streams >= 0)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CREATE TABLE IF NOT EXISTS public.catalog_valuations ( | |
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | |
| catalog_id UUID NOT NULL REFERENCES public.catalogs(id) ON DELETE CASCADE, | |
| low NUMERIC NOT NULL, -- band low, USD | |
| mid NUMERIC NOT NULL, -- band midpoint, USD | |
| high NUMERIC NOT NULL, -- band high, USD | |
| measured_song_count INTEGER NOT NULL, -- songs measured in the underlying capture | |
| total_streams BIGINT NOT NULL, -- whole-catalog lifetime streams at measurement | |
| measured_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), | |
| created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now() | |
| ); | |
| CREATE TABLE IF NOT EXISTS public.catalog_valuations ( | |
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | |
| catalog_id UUID NOT NULL REFERENCES public.catalogs(id) ON DELETE CASCADE, | |
| low NUMERIC NOT NULL, -- band low, USD | |
| mid NUMERIC NOT NULL, -- band midpoint, USD | |
| high NUMERIC NOT NULL, -- band high, USD | |
| measured_song_count INTEGER NOT NULL, -- songs measured in the underlying capture | |
| total_streams BIGINT NOT NULL, -- whole-catalog lifetime streams at measurement | |
| measured_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), | |
| created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), | |
| CONSTRAINT catalog_valuations_band_order_chk | |
| CHECK (low >= 0 AND low <= mid AND mid <= high), | |
| CONSTRAINT catalog_valuations_song_count_chk | |
| CHECK (measured_song_count >= 0), | |
| CONSTRAINT catalog_valuations_streams_chk | |
| CHECK (total_streams >= 0) | |
| ); |
🤖 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 `@supabase/migrations/20260729230000_create_catalog_valuations.sql` around
lines 21 - 31, Add table-level CHECK constraints to public.catalog_valuations
ensuring low, mid, and high are non-negative and ordered low <= mid <= high, and
measured_song_count and total_streams are non-negative. Keep the existing
columns and defaults unchanged.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## migration file excerpt"
if [ -f supabase/migrations/20260729230000_create_catalog_valuations.sql ]; then
cat -n supabase/migrations/20260729230000_create_catalog_valuations.sql
else
echo "migration file not found"
fi
echo
echo "## related catalog policy/RLS references"
rg -n "catalog_valuations|catalogs|RLS|enable row level security|ALTER TABLE .* ENABLE ROW LEVEL SECURITY|CREATE POLICY|GRANT .* catalog_valuations|pg_ls_rel_acl|supabase/functions|auth/user" -S supabase . -g '!node_modules' -g '!dist' -g '!build' | head -200
echo
echo "## SQL policy creation / grants candidates"
fd -e sql . supabase public supabase -x sh -c 'echo "--- $1"; rg -n "catalog_valuations|catalogs|ENABLE ROW LEVEL SECURITY|ALTER TABLE .* ENABLE ROW LEVEL SECURITY|CREATE POLICY|GRANT .* catalog|revoke all|role anon|role authenticated" "{}" -S' sh {}
echo
echo "## git diff stat/name/status"
git diff --stat || true
git diff --name-only || trueRepository: recoupable/database
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## catalogs migration"
cat -n supabase/migrations/20251005212508_create_catalogs_table.sql
echo
echo "## catalog-songs/measurements related migrations excerpts"
cat -n supabase/migrations/20260707120000_catalog_measurements_read_functions.sql
echo "---"
if [ -f supabase/migrations/20251005214038_create_catalog_songs_table.sql ]; then cat -n supabase/migrations/20251005214038_create_catalog_songs_table.sql; fi
echo "---"
if [ -f supabase/migrations/20251005210048_create_songs_table.sql ]; then sed -n '1,80p' supabase/migrations/20251005210048_create_songs_table.sql | cat -n; fi
echo
echo "## valuations route/API references"
rg -n "valuations|/catalogs/.*/valuations|catalogValuations|getCatalogValuations|runValuationHandler|catalog_id|catalogValuation" -S . \
-g '!node_modules' -g '!dist' -g '!build' | head -250
echo
echo "## table grants and RLS statements for public catalog tables (targeted)"
python3 - <<'PY'
from pathlib import Path
needle = ['catalogs', 'catalog_songs', 'catalog_valuations', 'account_catalogs']
for p in Path('supabase/migrations').glob('*.sql'):
s = p.read_text(errors='ignore')
hits = [key for key in needle if key in s.lower()]
if not hits:
continue
stmts = []
for line in s.splitlines():
l=line.lower().strip()
if any(key in l for key in hits):
stmts.append(line)
print(f'--- {p}')
for i, line in enumerate(s.splitlines(), 1):
if any(key in line.lower() for key in hits) or any(x in l_for_line for x in ['grant ', 'alter table', 'create policy', 'enable row level security', 'revoke ']):
print(f'{i:04d}: {line}')
print()
PYRepository: recoupable/database
Length of output: 11784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## account_catalogs migration excerpt"
sed -n '1,120p' supabase/migrations/20251005214926_create_account_catalogs_table.sql | cat -n
echo
echo "## valuations references across repo"
rg -n "catalog_valuation|catalogValuation|/valuations|valuations" -S . \
-g '!node_modules' -g '!dist' -g '!build' -g '!supabase/migrations/20260729230000_create_catalog_valuations.sql' | head -200
echo
echo "## all catalog_valuations references"
rg -n "catalog_valuations|catalog_valuation" -S . \
-g '!node_modules' -g '!dist' -g '!build' | head -250Repository: recoupable/database
Length of output: 3595
Grant catalog_valuations to the service-role endpoint or add RLS policies for authenticated access.
This table is documented as read by GET /api/catalogs/{catalogId}/valuations, but unlike the neighbor catalog tables, it has no grants/RLS. As written, the service-role writes can run and PostgREST queries will fail with the “new row violates row-level security policy” check on access-controlled tables, so add the intended grant/policy path here.
🤖 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 `@supabase/migrations/20260729230000_create_catalog_valuations.sql` around
lines 21 - 31, Update the catalog_valuations table definition to establish its
intended API access path, matching the neighboring catalog tables: either grant
the required privileges to the service-role endpoint or enable RLS and add
policies permitting authenticated reads for GET
/api/catalogs/{catalogId}/valuations. Ensure service-role writes remain
permitted and the catalog_id scope is enforced consistently with existing
policies.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
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="supabase/migrations/20260729230000_create_catalog_valuations.sql">
<violation number="1" location="supabase/migrations/20260729230000_create_catalog_valuations.sql:30">
P2: The table only enforces NOT NULL on low/mid/high, measured_song_count, and total_streams. Nothing stops a writer from inserting negative counts/streams or an invalid band where low > mid or mid > high. Consider adding CHECK constraints (e.g. CHECK (low >= 0 AND low <= mid AND mid <= high), CHECK (measured_song_count >= 0), CHECK (total_streams >= 0)) so the schema itself preserves the valuation history contract.</violation>
<violation number="2" location="supabase/migrations/20260729230000_create_catalog_valuations.sql:35">
P1: Catalog valuation history can be read or modified through the public data API without catalog-level authorization. Enable RLS here (the API's service role still bypasses it), matching `catalogs` and the measurement store.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| -- The read is always "latest rows for one catalog". | ||
| CREATE INDEX IF NOT EXISTS catalog_valuations_catalog_measured_idx | ||
| ON public.catalog_valuations (catalog_id, measured_at DESC); |
There was a problem hiding this comment.
P1: Catalog valuation history can be read or modified through the public data API without catalog-level authorization. Enable RLS here (the API's service role still bypasses it), matching catalogs and the measurement store.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260729230000_create_catalog_valuations.sql, line 35:
<comment>Catalog valuation history can be read or modified through the public data API without catalog-level authorization. Enable RLS here (the API's service role still bypasses it), matching `catalogs` and the measurement store.</comment>
<file context>
@@ -0,0 +1,35 @@
+
+-- The read is always "latest rows for one catalog".
+CREATE INDEX IF NOT EXISTS catalog_valuations_catalog_measured_idx
+ ON public.catalog_valuations (catalog_id, measured_at DESC);
</file context>
| measured_song_count INTEGER NOT NULL, -- songs measured in the underlying capture | ||
| total_streams BIGINT NOT NULL, -- whole-catalog lifetime streams at measurement | ||
| measured_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), | ||
| created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now() |
There was a problem hiding this comment.
P2: The table only enforces NOT NULL on low/mid/high, measured_song_count, and total_streams. Nothing stops a writer from inserting negative counts/streams or an invalid band where low > mid or mid > high. Consider adding CHECK constraints (e.g. CHECK (low >= 0 AND low <= mid AND mid <= high), CHECK (measured_song_count >= 0), CHECK (total_streams >= 0)) so the schema itself preserves the valuation history contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260729230000_create_catalog_valuations.sql, line 30:
<comment>The table only enforces NOT NULL on low/mid/high, measured_song_count, and total_streams. Nothing stops a writer from inserting negative counts/streams or an invalid band where low > mid or mid > high. Consider adding CHECK constraints (e.g. CHECK (low >= 0 AND low <= mid AND mid <= high), CHECK (measured_song_count >= 0), CHECK (total_streams >= 0)) so the schema itself preserves the valuation history contract.</comment>
<file context>
@@ -0,0 +1,35 @@
+ measured_song_count INTEGER NOT NULL, -- songs measured in the underlying capture
+ total_streams BIGINT NOT NULL, -- whole-catalog lifetime streams at measurement
+ measured_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(),
+ created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now()
+);
+
</file context>
This was referenced Jul 29, 2026
Contributor
Author
Pre-merge verification (2026-07-29)
No data manipulation in this migration; on merge the Supabase GitHub integration applies it to prod. Post-merge check will be the same |
Contributor
Author
Post-merge verification (2026-07-29, prod)Supabase check on merge commit
Matches the pre-merge dry-run exactly. Chain unblocked: api#795 is next. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Database leg of row 15, the keystone on chat#1889. Merge order: docs#279 → this → api#TBD.
catalog_valuations: one row per band computation —catalog_id(FK CASCADE),low/mid/high(USD),measured_song_count,total_streams,measured_at— with a(catalog_id, measured_at DESC)index matching the only read shape (latest-first per catalog). History table by design, never columns oncatalogs: the whole point is two numbers to compare.Writers land in the api PR:
runValuationHandler(always) + the measurements read path (at most one row per catalog per day). Schema-only migration — no data touched; trivially fresh-env safe.🤖 Generated with Claude Code
Summary by cubic
Add persisted valuation history per catalog via a new catalog_valuations table so we can compare values over time (chat#1889, row 15). Optimized for latest‑first reads; writers will be added in the API follow-up.
New Features
Migration
Written for commit 76c03c0. Summary will update on new commits.
Summary by CodeRabbit