Merged
Conversation
|
Cursor Agent can help with this pull request. Just |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughUpdates playlist ID input handling across TypeScript, workflow configuration, and documentation by adding input normalization to trim empty values and implementing fallback logic that prioritizes repository variables or secrets before defaulting to a standard playlist ID. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 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.
🧹 Nitpick comments (2)
other/semantic-search/__tests__/youtube-playlist.test.ts (2)
20-28: Add a regression test for the exact GH Actions empty-string scenario.Test 3 validates whitespace-only
playlistUrlEnv(' '), and test 4 validates both env vars as''. The specific PR bug — GH Actions emitting''forYOUTUBE_PLAYLIST_URLwhileYOUTUBE_PLAYLIST_IDis non-empty — falls through the gap.➕ Suggested additional test
+ test('resolveYouTubePlaylistInput ignores empty-string URL env (GitHub Actions default) and uses playlist ID', () => { + const input = resolveYouTubePlaylistInput({ + playlistArg: undefined, + playlistUrlEnv: '', // GitHub Actions sets undefined repo vars to '' + playlistIdEnv: 'PL1234567890', + defaultPlaylistId: 'PLDEFAULT1234', + }) + expect(input).toBe('PL1234567890') + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/semantic-search/__tests__/youtube-playlist.test.ts` around lines 20 - 28, The test suite is missing a regression test for the GitHub Actions behavior where an env var is the literal two-character string "''"; add a new unit test in youtube-playlist.test.ts that calls resolveYouTubePlaylistInput with playlistArg: undefined, playlistUrlEnv: "''" (literal), playlistIdEnv: e.g. 'PL1234567890', and a defaultPlaylistId, then assert the return equals the playlistIdEnv; reference the existing test pattern used around resolveYouTubePlaylistInput to mirror naming and expectations so this specific GH Actions empty-string scenario is covered.
7-38: Consider adding tests forplaylistArgpriority andgetYouTubePlaylistIdnull/invalid returns.Two gaps:
resolveYouTubePlaylistInputhas three inputs in priority order (playlistArg>playlistUrlEnv>playlistIdEnv), but no test exercisesplaylistArgwinning over the env vars.getYouTubePlaylistIdhas no tests fornull/undefined/empty inputs or an invalid string (all should returnnull).➕ Suggested additional tests
+ test('resolveYouTubePlaylistInput uses playlistArg over env values', () => { + const input = resolveYouTubePlaylistInput({ + playlistArg: 'PL_ARG1234567', + playlistUrlEnv: 'https://www.youtube.com/playlist?list=PLURL1234567', + playlistIdEnv: 'PL_ENV1234567', + defaultPlaylistId: 'PLDEFAULT1234', + }) + expect(input).toBe('PL_ARG1234567') + }) + + test('getYouTubePlaylistId returns null for null/undefined/empty/invalid inputs', () => { + expect(getYouTubePlaylistId(null)).toBeNull() + expect(getYouTubePlaylistId(undefined)).toBeNull() + expect(getYouTubePlaylistId('')).toBeNull() + expect(getYouTubePlaylistId(' ')).toBeNull() + expect(getYouTubePlaylistId('not-a-valid-id-or-url')).toBeNull() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/semantic-search/__tests__/youtube-playlist.test.ts` around lines 7 - 38, Add tests covering (1) resolveYouTubePlaylistInput prioritization by asserting that when playlistArg is provided it is returned even if playlistUrlEnv and playlistIdEnv are set, and (2) getYouTubePlaylistId handling of invalid/empty inputs by asserting it returns null for inputs like undefined, null, empty string, or a non-playlist string; add these test cases to the existing suite using the functions getYouTubePlaylistId and resolveYouTubePlaylistInput so the behavior is explicitly verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@other/semantic-search/__tests__/youtube-playlist.test.ts`:
- Around line 20-28: The test suite is missing a regression test for the GitHub
Actions behavior where an env var is the literal two-character string "''"; add
a new unit test in youtube-playlist.test.ts that calls
resolveYouTubePlaylistInput with playlistArg: undefined, playlistUrlEnv: "''"
(literal), playlistIdEnv: e.g. 'PL1234567890', and a defaultPlaylistId, then
assert the return equals the playlistIdEnv; reference the existing test pattern
used around resolveYouTubePlaylistInput to mirror naming and expectations so
this specific GH Actions empty-string scenario is covered.
- Around line 7-38: Add tests covering (1) resolveYouTubePlaylistInput
prioritization by asserting that when playlistArg is provided it is returned
even if playlistUrlEnv and playlistIdEnv are set, and (2) getYouTubePlaylistId
handling of invalid/empty inputs by asserting it returns null for inputs like
undefined, null, empty string, or a non-playlist string; add these test cases to
the existing suite using the functions getYouTubePlaylistId and
resolveYouTubePlaylistInput so the behavior is explicitly verified.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
other/semantic-search/__tests__/youtube-playlist.test.tsother/semantic-search/index-youtube-playlist.tsother/semantic-search/youtube-playlist.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
other/semantic-search/index-youtube-playlist.ts (1)
295-317:getYouTubePlaylistIdduplicatesparsePlaylistIdfromapp/routes/youtube.tsxThe two functions are functionally identical (same regex, same URL-parsing fallback, same return type);
DEFAULT_PLAYLIST_IDis also hard-coded to the same literal in both files. The PR summary mentions extracting these into a sharedyoutube-playlist.ts, but that module doesn't appear in the PR. Consider creating it to avoid drift:♻️ Suggested shared module sketch
// other/semantic-search/youtube-playlist.ts (new file) +export const DEFAULT_PLAYLIST_ID = 'PLV5CVI1eNcJgNqzNwcs4UKrlJdhfDjshf' + +export function parsePlaylistId(value: string | undefined | null): string | null { + if (!value) return null + const trimmed = value.trim() + if (!trimmed) return null + if (/^[A-Za-z0-9_-]{10,}$/.test(trimmed)) return trimmed + try { + const url = new URL(trimmed) + const list = url.searchParams.get('list') + return list && /^[A-Za-z0-9_-]{10,}$/.test(list) ? list : null + } catch { + return null + } +}Then import in both
index-youtube-playlist.tsandapp/routes/youtube.tsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@other/semantic-search/index-youtube-playlist.ts` around lines 295 - 317, Duplicate playlist parsing exists: getYouTubePlaylistId here and parsePlaylistId in app/routes/youtube.tsx (and the same DEFAULT_PLAYLIST_ID literal). Create a shared module (e.g., youtube-playlist.ts) that exports a single parsePlaylistId (or getYouTubePlaylistId) function and DEFAULT_PLAYLIST_ID constant implementing the existing regex + URL fallback logic, then replace the local implementations in index-youtube-playlist.ts and app/routes/youtube.tsx with imports from that shared module to remove duplication and centralize the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@other/semantic-search/index-youtube-playlist.ts`:
- Around line 295-317: Duplicate playlist parsing exists: getYouTubePlaylistId
here and parsePlaylistId in app/routes/youtube.tsx (and the same
DEFAULT_PLAYLIST_ID literal). Create a shared module (e.g., youtube-playlist.ts)
that exports a single parsePlaylistId (or getYouTubePlaylistId) function and
DEFAULT_PLAYLIST_ID constant implementing the existing regex + URL fallback
logic, then replace the local implementations in index-youtube-playlist.ts and
app/routes/youtube.tsx with imports from that shared module to remove
duplication and centralize the behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.env.example.github/workflows/index-semantic-youtube.ymlapp/routes/youtube.tsxother/semantic-search/index-youtube-playlist.tsother/semantic-search/readme.md
✅ Files skipped from review due to trivial changes (1)
- other/semantic-search/readme.md
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
bot
force-pushed
the
cursor/youtube-playlist-id-configuration-993d
branch
from
9065e18 to
7c784f5
Compare
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
2 participants
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.
Fix YouTube re-indexing job failure by treating empty
YOUTUBE_PLAYLIST_URLas unset, allowingYOUTUBE_PLAYLIST_IDto be used.GitHub Actions sets undefined repo variables to empty strings. The existing script's nullish coalescing operator (
??) would then select this empty string forYOUTUBE_PLAYLIST_URLinstead of falling back toYOUTUBE_PLAYLIST_IDor the default, causing the job to fail.Note
Low Risk
Low risk configuration hardening: only affects how the YouTube indexing job selects
YOUTUBE_PLAYLIST_ID, with no changes to indexing logic or data formats.Overview
Hardens YouTube semantic-search indexing configuration so empty values don’t break scheduled runs.
The GitHub Actions workflow now falls back from
vars.YOUTUBE_PLAYLIST_IDtosecrets.YOUTUBE_PLAYLIST_ID. The YouTube playlist indexer treats empty/whitespace--playlistorYOUTUBE_PLAYLIST_IDas unset (vianonEmptyTrimmed) before falling back to the built-in default, and updates the error message/docs to reflect the env var option.Written by Cursor Bugbot for commit 7c784f5. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Documentation
Chores