agent: @U0AJM7X8FBR # Song Filtering for Content Creation PipelineProblemThe co#111
Open
sweetmantech wants to merge 1 commit intomainfrom
Open
agent: @U0AJM7X8FBR # Song Filtering for Content Creation PipelineProblemThe co#111sweetmantech wants to merge 1 commit intomainfrom
sweetmantech wants to merge 1 commit intomainfrom
Conversation
Adds an optional `songs` array to the content creation payload. When provided, only songs matching the given slugs are eligible for selection. When omitted, existing behavior (all songs) is preserved. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request adds optional song filtering to the content creation pipeline. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/content/__tests__/selectAudioClip.test.ts`:
- Around line 58-119: Add a test to the "song filtering" suite verifying that
passing songs: [] to selectAudioClip is treated the same as omitting the songs
field (i.e., no filter): mock listArtistSongs to return allSongPaths, call
selectAudioClip with songs: [], then assert behavior consistent with the first
test (e.g., vi.mocked(listArtistSongs) was called and selectAudioClip resolves
and/or fetchGithubFile is called for any eligible song); use the existing test
helpers and the same structure as the other tests so this edge case is covered.
In `@src/content/selectAudioClip.ts`:
- Around line 67-74: The code currently only applies the songs filter when
songs.length > 0, which causes an explicit empty songs array to be ignored;
change the condition to detect the presence of the songs parameter (e.g., if
(songs !== undefined && songs !== null) or if (Array.isArray(songs))) so that
songPaths is always filtered against the provided songs array (even when empty),
and keep the existing throw when songPaths.length === 0; update the block around
the songPaths filtering in selectAudioClip (variables: songs, songPaths)
accordingly.
🪄 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
Run ID: 641587a6-7974-4f0a-b7ec-72f6b37058e6
📒 Files selected for processing (5)
src/content/__tests__/selectAudioClip.test.tssrc/content/selectAudioClip.tssrc/schemas/__tests__/contentCreationSchema.test.tssrc/schemas/contentCreationSchema.tssrc/tasks/createContentTask.ts
Comment on lines
+58
to
+119
| describe("song filtering", () => { | ||
| it("picks from all songs when songs is omitted", async () => { | ||
| vi.mocked(listArtistSongs).mockResolvedValue(allSongPaths); | ||
|
|
||
| await selectAudioClip({ | ||
| githubRepo: "https://github.com/org/repo", | ||
| artistSlug: "test-artist", | ||
| clipDuration: 15, | ||
| lipsync: false, | ||
| }); | ||
|
|
||
| // listArtistSongs was called — all 3 are eligible | ||
| expect(vi.mocked(listArtistSongs)).toHaveBeenCalledOnce(); | ||
| }); | ||
|
|
||
| it("filters to specified songs when songs array is provided", async () => { | ||
| vi.mocked(listArtistSongs).mockResolvedValue(allSongPaths); | ||
|
|
||
| await selectAudioClip({ | ||
| githubRepo: "https://github.com/org/repo", | ||
| artistSlug: "test-artist", | ||
| clipDuration: 15, | ||
| lipsync: false, | ||
| songs: ["adhd"], | ||
| }); | ||
|
|
||
| // Only adhd.mp3 is eligible — transcribeSong should be called with adhd path | ||
| expect(vi.mocked(fetchGithubFile)).toHaveBeenCalledWith( | ||
| expect.any(String), | ||
| "artists/test-artist/songs/adhd/adhd.mp3", | ||
| ); | ||
| }); | ||
|
|
||
| it("throws when none of the specified songs are found", async () => { | ||
| vi.mocked(listArtistSongs).mockResolvedValue(allSongPaths); | ||
|
|
||
| await expect( | ||
| selectAudioClip({ | ||
| githubRepo: "https://github.com/org/repo", | ||
| artistSlug: "test-artist", | ||
| clipDuration: 15, | ||
| lipsync: false, | ||
| songs: ["nonexistent-song"], | ||
| }), | ||
| ).rejects.toThrow("None of the specified songs [nonexistent-song] were found"); | ||
| }); | ||
|
|
||
| it("filters to multiple specified songs", async () => { | ||
| vi.mocked(listArtistSongs).mockResolvedValue(allSongPaths); | ||
|
|
||
| // Should not throw — both adhd and hiccups exist | ||
| await expect( | ||
| selectAudioClip({ | ||
| githubRepo: "https://github.com/org/repo", | ||
| artistSlug: "test-artist", | ||
| clipDuration: 15, | ||
| lipsync: false, | ||
| songs: ["adhd", "hiccups"], | ||
| }), | ||
| ).resolves.toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add a regression test for songs: [] semantics.
Given Line 67 in src/content/selectAudioClip.ts, an explicit empty array currently behaves like “no filter.” Please add a test that locks the intended behavior for songs: [] so this edge case doesn’t regress.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/content/__tests__/selectAudioClip.test.ts` around lines 58 - 119, Add a
test to the "song filtering" suite verifying that passing songs: [] to
selectAudioClip is treated the same as omitting the songs field (i.e., no
filter): mock listArtistSongs to return allSongPaths, call selectAudioClip with
songs: [], then assert behavior consistent with the first test (e.g.,
vi.mocked(listArtistSongs) was called and selectAudioClip resolves and/or
fetchGithubFile is called for any eligible song); use the existing test helpers
and the same structure as the other tests so this edge case is covered.
Comment on lines
+67
to
+74
| if (songs && songs.length > 0) { | ||
| songPaths = songPaths.filter(path => | ||
| songs.some(slug => path.includes(`/songs/${slug}/`)), | ||
| ); | ||
| if (songPaths.length === 0) { | ||
| throw new Error(`None of the specified songs [${songs.join(", ")}] were found`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Honor explicit empty songs input instead of falling back to all songs.
Line 67 currently skips filtering when songs is [], so an explicit filter can unintentionally select any song. That breaks the payload contract where provided songs should constrain eligibility.
💡 Suggested fix
- if (songs && songs.length > 0) {
- songPaths = songPaths.filter(path =>
- songs.some(slug => path.includes(`/songs/${slug}/`)),
- );
+ if (songs !== undefined) {
+ const requestedSlugs = songs.map(slug => slug.trim()).filter(Boolean);
+ if (requestedSlugs.length === 0) {
+ throw new Error("songs must include at least one non-empty slug when provided");
+ }
+
+ songPaths = songPaths.filter(path =>
+ requestedSlugs.some(slug => path.includes(`/songs/${slug}/`)),
+ );
if (songPaths.length === 0) {
- throw new Error(`None of the specified songs [${songs.join(", ")}] were found`);
+ throw new Error(`None of the specified songs [${requestedSlugs.join(", ")}] were found`);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/content/selectAudioClip.ts` around lines 67 - 74, The code currently only
applies the songs filter when songs.length > 0, which causes an explicit empty
songs array to be ignored; change the condition to detect the presence of the
songs parameter (e.g., if (songs !== undefined && songs !== null) or if
(Array.isArray(songs))) so that songPaths is always filtered against the
provided songs array (even when empty), and keep the existing throw when
songPaths.length === 0; update the block around the songPaths filtering in
selectAudioClip (variables: songs, songPaths) accordingly.
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.
Automated PR from coding agent.
Summary by CodeRabbit
New Features
Tests