Fix catalog filtering for small catalogs and bound large results - #914
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: recoupable/api/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 5/5
lib/catalog/__tests__/analyzeFullCatalog.test.tsexceeds the 100-line style limit, which modestly reduces test maintainability; extract thecatalog()andselect()helpers as suggested.
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/__tests__/analyzeFullCatalog.test.ts">
<violation number="1" location="lib/catalog/__tests__/analyzeFullCatalog.test.ts:42">
P3: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
This test file is 111 lines long, exceeding the 100-line limit in the code-style rule. Extract the `catalog()` and `select()` helpers into a shared test helper file (or split the regression tests) to bring the file under 100 lines while keeping the suite cohesive.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as API Client
participant Route as Catalog Route
participant Analyze as analyzeFullCatalog
participant DB as Supabase DB
participant Refine as refineResults
participant Model as AI Model (generateObject)
Note over Client,Model: Catalog Filtering Flow
Client->>Route: Request catalog analysis with criteria
Route->>Analyze: analyzeFullCatalog({catalogId, criteria})
Analyze->>DB: selectCatalogSongsWithArtists(page 1)
DB-->>Analyze: Page 1 songs + total_count
Analyze->>DB: selectCatalogSongsWithArtists(page N)
DB-->>Analyze: Page N songs + total_count
Analyze->>Analyze: Flatten all pages into allSongs
alt allSongs is empty
Analyze-->>Route: { results: [], totalSongs: 0, totalPages: 0 }
Note over Analyze,Model: No inference performed for empty catalogs
else allSongs non-empty
Analyze->>Refine: refineResults(allSongs, criteria)
loop For each batch of 100 songs in parallel
Refine->>Model: generateObject(songs batch, criteria)
Model-->>Refine: selected_song_isrcs
end
Refine->>Refine: Deduplicate and validate ISRCs against catalog
Refine->>Refine: Cap results at first 1000 in catalog order
Refine-->>Analyze: Filtered results up to 1000
Analyze-->>Route: { results, totalSongs, totalPages }
end
alt Inference error occurs
Model-->>Refine: Error thrown
Refine-->>Analyze: Propagate error
Analyze-->>Route: 500 error
Note over Refine,Analyze: Errors propagate - no unfiltered fallback
end
Route-->>Client: Response with filtered results
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -0,0 +1,111 @@ | |||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | |||
There was a problem hiding this comment.
P3: Custom agent: Enforce Clear Code Style and Maintainability Practices
This test file is 111 lines long, exceeding the 100-line limit in the code-style rule. Extract the catalog() and select() helpers into a shared test helper file (or split the regression tests) to bring the file under 100 lines while keeping the suite cohesive.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/catalog/__tests__/analyzeFullCatalog.test.ts, line 42:
<comment>This test file is 111 lines long, exceeding the 100-line limit in the code-style rule. Extract the `catalog()` and `select()` helpers into a shared test helper file (or split the regression tests) to bring the file under 100 lines while keeping the suite cohesive.</comment>
<file context>
@@ -0,0 +1,111 @@
+ } as Awaited<ReturnType<typeof generateObject>>);
+}
+
+describe("analyzeFullCatalog filtering", () => {
+ beforeEach(() => vi.resetAllMocks());
+
</file context>
Catalog requests with 1,000 songs or fewer previously returned the entire catalog without applying the criteria. Larger catalogs repeatedly invoked AI filtering until fewer than 1,001 songs remained, which could continue indefinitely when all songs matched.
Filter every nonempty catalog in one pass, then cap matching results at 1,000 in catalog order. All pages are considered before limiting. This is a deterministic cap, not a new global relevance ranking; the model and response shape are unchanged. Empty catalogs avoid inference, and inference errors propagate rather than returning unfiltered songs.
Validation:
Small catalogs now incur the intended model calls (one per 100-song batch). Large catalogs retain the existing parallel batching. No production inference or deployment was performed.
Summary by cubic
Fixes catalog filtering so catalogs of 1,000 songs or fewer are no longer returned unfiltered, and large all-matching catalogs no longer loop forever. Filter every nonempty catalog once, then cap matches at 1,000 in catalog order; the cap is deterministic, and the model and response shape are unchanged.
Validation
Written for commit ff33b77. Summary will update on new commits.