Conversation
…en animation direction - Implemented `Root.jsx` to define the main composition for the lyrics video using Remotion. - Created `index.jsx` to register the root component for rendering. - Developed `ai-director.js` to handle AI-based animation direction generation with OpenAI and GLM-5 models. - Added `jiosaavn-client.js` for fetching song metadata from JioSaavn API. - Introduced `lyrics-client.js` to retrieve lyrics from multiple sources with fallback mechanisms. - Created `lyrics-transform.js` to build timelines from synced and plain lyrics. - Implemented `phase3-fetch-song-lyrics.js` to fetch song metadata and lyrics, generating a structured JSON output. - Developed `phase4-generate-direction.js` to generate animation direction based on fetched lyrics. - Added `render-test-video.js` to render a test video using the generated lyrics and animation direction.
|
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: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds a full end-to-end lyric-driven video pipeline: song discovery and lyrics fetch, AI-driven animation direction, Remotion-based composition and rendering, optional YouTube upload, CLI scripts, CI workflows, environment configuration, and sample data/artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Scripts
participant JioSaavn as JioSaavn API
participant LyricsAPI as Lyrics APIs
participant AIProvider as AI Provider<br/>(Pollinations / GLM-5)
participant Bundler as Remotion Bundler
participant Renderer as Remotion Renderer
participant YouTube as YouTube API
User->>CLI: run phase3 (query or song-id)
CLI->>JioSaavn: resolve song metadata
JioSaavn-->>CLI: song details
CLI->>LyricsAPI: fetch lyrics (LRCLib → Lyrics.ovh fallback)
LyricsAPI-->>CLI: synced/plain lyrics
CLI->>CLI: build timeline (phase3 JSON)
User->>CLI: run phase4 (input phase3 JSON)
CLI->>AIProvider: probe & request animation direction
AIProvider-->>CLI: direction JSON
CLI->>CLI: normalize/validate direction (phase4 JSON)
User->>CLI: run phase5 (render)
CLI->>Bundler: bundle Remotion entry
Bundler-->>CLI: bundle URL
CLI->>Renderer: render composition (LyricsVideo) → MP4
Renderer-->>CLI: output MP4
User->>CLI: run phase7 (upload) [optional]
CLI->>YouTube: (dry-run?) or OAuth upload
YouTube-->>CLI: upload response (videoId / URL)
CLI-->>User: upload JSON artifact
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Node/Remotion-based pipeline to generate and render animated lyrics videos, including scripts to (1) fetch song metadata + lyrics, (2) generate AI-driven animation direction, and (3) render a test video composition.
Changes:
- Added a Remotion project (
remotion/*) with aLyricsVideocomposition and a Node script to bundle/render it. - Added “phase 3/4” CLI scripts and supporting libraries to fetch lyrics (with fallback) and generate animation direction via OpenAI-compatible providers.
- Added workflow + sample data files to exercise the pipeline and upload a rendered MP4 artifact.
Reviewed changes
Copilot reviewed 17 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/render-test-video.js |
Bundles the Remotion entry and renders LyricsVideo to output/test-video.mp4. |
scripts/phase3-fetch-song-lyrics.js |
Fetches JioSaavn song metadata + lyrics and emits a structured timeline JSON. |
scripts/phase4-generate-direction.js |
Generates AI “animation direction” JSON from a phase-3 lyrics timeline. |
scripts/lib/lyrics-transform.js |
Converts synced LRC (or approximated plain lyrics) into a timed line timeline. |
scripts/lib/lyrics-client.js |
Fetches lyrics via LRCLIB first, then falls back to lyrics.ovh. |
scripts/lib/jiosaavn-client.js |
Resolves song metadata via multiple JioSaavn API endpoints with retries. |
scripts/lib/ai-director.js |
Calls OpenAI-compatible endpoints and normalizes AI output into direction + per-line instructions. |
remotion/index.jsx |
Registers the Remotion root component for rendering. |
remotion/Root.jsx |
Defines the LyricsVideo composition (id, dimensions, fps, defaults). |
remotion/LyricsVideo.jsx |
Renders timed lyric lines with simple fade/translate animations. |
package.json |
Adds Node scripts for phase 3/4 + render, and introduces Remotion/React dependencies. |
package-lock.json |
Locks new dependency graph for Remotion/renderer/bundler and transitive packages. |
output/.gitkeep |
Keeps the output/ directory tracked for generated renders/artifacts. |
data/test-lyrics.json |
Minimal fixture for rendering a short test video. |
data/phase3-lyrics.json |
Sample generated phase-3 output (song metadata + synced lyric timeline). |
data/phase3-lyrics-fallback.json |
Sample generated phase-3 output using approximate timing (fallback provider). |
data/phase4-direction.json |
Sample generated phase-4 output (AI direction + scene/line directions). |
.gitignore |
Ignores node_modules/, .env, and rendered MP4 outputs. |
.github/workflows/test-render.yml |
Adds a manual workflow to render and upload output/test-video.mp4. |
.env.example |
Documents environment variables for the phase-4 AI provider setup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "phase3:build": "node scripts/phase3-fetch-song-lyrics.js", | ||
| "phase4:direction": "node scripts/phase4-generate-direction.js", | ||
| "render:test": "node scripts/render-test-video.js", | ||
| "test": "npm run render:test" |
There was a problem hiding this comment.
The test script is currently mapped to a full Remotion render (npm run render:test). This makes npm test slow and side-effectful (writes an output video, requires rendering deps), which can be surprising for local dev and CI tooling that assumes tests are quick/deterministic. Consider keeping render:test as-is and reserving test for unit/lint checks (or a lightweight smoke test).
Suggested change
| "test": "npm run render:test" | |
| "test": "node -e \"console.log('No automated tests configured');\"" |
Comment on lines
+18
to
+19
| console.log('[phase-1] Loading test lyrics JSON...'); | ||
| const inputProps = await readInputProps(); |
There was a problem hiding this comment.
Log prefixes are labeled [phase-1], but this file is a standalone renderer (render-test-video.js) and other scripts use [phase-3]/[phase-4]. The mismatch makes logs harder to correlate. Consider renaming the prefix to match the script purpose (e.g., [render-test]) or the actual pipeline phase.
| @@ -0,0 +1,32 @@ | |||
| name: Phase 2 Test Render | |||
There was a problem hiding this comment.
Workflow name refers to "Phase 2", but the pipeline scripts added in this PR are phase-3/phase-4. Consider aligning the workflow name with the actual phase/purpose of the job to avoid confusion.
| - name: Upload rendered video artifact | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: phase-2-test-video |
There was a problem hiding this comment.
The uploaded artifact is named phase-2-test-video, but the job runs npm run render:test (and the rest of the pipeline uses phase-3/4 naming). Consider renaming the artifact to match the job/script so it’s easier to find and relate to logs.
Suggested change
| name: phase-2-test-video | |
| name: render-test-video |
Comment on lines
+6
to
+69
| "songSelectionMethod": "search-best-match", | ||
| "sourceApis": { | ||
| "jiosaavn": "https://jiosaavn-api-taupe-phi.vercel.app/api", | ||
| "lyrics": "lrclib", | ||
| "lyricsMatchMode": "direct-get" | ||
| } | ||
| }, | ||
| "song": { | ||
| "id": "Q6l0a09y", | ||
| "name": "Aaj Ki Raat", | ||
| "artist": "Amitabh Bhattacharya, Sachin-Jigar, Madhubanti Bagchi, Divya Kumar", | ||
| "album": "Stree 2", | ||
| "duration": 228, | ||
| "language": "hindi", | ||
| "year": "2024", | ||
| "url": "https://www.jiosaavn.com/song/aaj-ki-raat/IV4HARUADko", | ||
| "image": "https://c.saavncdn.com/373/Stree-2-Hindi-2024-20240828083834-500x500.jpg", | ||
| "hasLyrics": true, | ||
| "source": "jiosaavn" | ||
| }, | ||
| "lyricsRaw": { | ||
| "provider": "lrclib", | ||
| "hasSyncedLyrics": true, | ||
| "hasPlainLyrics": true, | ||
| "reference": { | ||
| "id": 12247214, | ||
| "trackName": "Aaj Ki Raat", | ||
| "artistName": "Amitabh Bhattacharya", | ||
| "albumName": "Stree 2 (Original Motion Picture Soundtrack)" | ||
| } | ||
| }, | ||
| "lines": [ | ||
| { | ||
| "text": "थोड़ी फ़ुर्सत भी, मेरी जाँ, कभी बाँहों को दीजिए", | ||
| "start": 9.44, | ||
| "end": 17.59 | ||
| }, | ||
| { | ||
| "text": "थोड़ी फ़ुर्सत भी, मेरी जाँ, कभी बाँहों को दीजिए", | ||
| "start": 17.59, | ||
| "end": 25.74 | ||
| }, | ||
| { | ||
| "text": "आज की रात मज़ा हुस्न का आँखों से लीजिए", | ||
| "start": 25.74, | ||
| "end": 33.94 | ||
| }, | ||
| { | ||
| "text": "आज की रात मज़ा हुस्न का आँखों से लीजिए", | ||
| "start": 33.94, | ||
| "end": 42.29 | ||
| }, | ||
| { | ||
| "text": "वक़्त बर्बाद ना बिन बात की बातों में कीजिए", | ||
| "start": 42.29, | ||
| "end": 50.49 | ||
| }, | ||
| { | ||
| "text": "वक़्त बर्बाद ना बिन बात की बातों में कीजिए", | ||
| "start": 50.49, | ||
| "end": 58.95 | ||
| }, | ||
| { | ||
| "text": "आज की रात मज़ा हुस्न का आँखों से लीजिए", |
There was a problem hiding this comment.
This file commits full, real song lyrics into the repository. That can create IP/licensing concerns and also makes the repo heavier/noisier as generated artifacts accumulate. Consider removing this from version control and generating it locally (or replacing it with a minimal synthetic fixture like data/test-lyrics.json).
Suggested change
| "songSelectionMethod": "search-best-match", | |
| "sourceApis": { | |
| "jiosaavn": "https://jiosaavn-api-taupe-phi.vercel.app/api", | |
| "lyrics": "lrclib", | |
| "lyricsMatchMode": "direct-get" | |
| } | |
| }, | |
| "song": { | |
| "id": "Q6l0a09y", | |
| "name": "Aaj Ki Raat", | |
| "artist": "Amitabh Bhattacharya, Sachin-Jigar, Madhubanti Bagchi, Divya Kumar", | |
| "album": "Stree 2", | |
| "duration": 228, | |
| "language": "hindi", | |
| "year": "2024", | |
| "url": "https://www.jiosaavn.com/song/aaj-ki-raat/IV4HARUADko", | |
| "image": "https://c.saavncdn.com/373/Stree-2-Hindi-2024-20240828083834-500x500.jpg", | |
| "hasLyrics": true, | |
| "source": "jiosaavn" | |
| }, | |
| "lyricsRaw": { | |
| "provider": "lrclib", | |
| "hasSyncedLyrics": true, | |
| "hasPlainLyrics": true, | |
| "reference": { | |
| "id": 12247214, | |
| "trackName": "Aaj Ki Raat", | |
| "artistName": "Amitabh Bhattacharya", | |
| "albumName": "Stree 2 (Original Motion Picture Soundtrack)" | |
| } | |
| }, | |
| "lines": [ | |
| { | |
| "text": "थोड़ी फ़ुर्सत भी, मेरी जाँ, कभी बाँहों को दीजिए", | |
| "start": 9.44, | |
| "end": 17.59 | |
| }, | |
| { | |
| "text": "थोड़ी फ़ुर्सत भी, मेरी जाँ, कभी बाँहों को दीजिए", | |
| "start": 17.59, | |
| "end": 25.74 | |
| }, | |
| { | |
| "text": "आज की रात मज़ा हुस्न का आँखों से लीजिए", | |
| "start": 25.74, | |
| "end": 33.94 | |
| }, | |
| { | |
| "text": "आज की रात मज़ा हुस्न का आँखों से लीजिए", | |
| "start": 33.94, | |
| "end": 42.29 | |
| }, | |
| { | |
| "text": "वक़्त बर्बाद ना बिन बात की बातों में कीजिए", | |
| "start": 42.29, | |
| "end": 50.49 | |
| }, | |
| { | |
| "text": "वक़्त बर्बाद ना बिन बात की बातों में कीजिए", | |
| "start": 50.49, | |
| "end": 58.95 | |
| }, | |
| { | |
| "text": "आज की रात मज़ा हुस्न का आँखों से लीजिए", | |
| "songSelectionMethod": "synthetic-fixture", | |
| "sourceApis": { | |
| "jiosaavn": "synthetic", | |
| "lyrics": "synthetic", | |
| "lyricsMatchMode": "fixture" | |
| } | |
| }, | |
| "song": { | |
| "id": "test-song-001", | |
| "name": "Test Song", | |
| "artist": "Sample Artist", | |
| "album": "Sample Album", | |
| "duration": 228, | |
| "language": "test", | |
| "year": "2026", | |
| "url": "https://example.com/test-song", | |
| "image": "https://example.com/test-song.jpg", | |
| "hasLyrics": true, | |
| "source": "synthetic" | |
| }, | |
| "lyricsRaw": { | |
| "provider": "synthetic", | |
| "hasSyncedLyrics": true, | |
| "hasPlainLyrics": true, | |
| "reference": { | |
| "id": 1, | |
| "trackName": "Test Song", | |
| "artistName": "Sample Artist", | |
| "albumName": "Sample Album" | |
| } | |
| }, | |
| "lines": [ | |
| { | |
| "text": "Sample lyric line 1", | |
| "start": 9.44, | |
| "end": 17.59 | |
| }, | |
| { | |
| "text": "Sample lyric line 2", | |
| "start": 17.59, | |
| "end": 25.74 | |
| }, | |
| { | |
| "text": "Sample lyric line 3", | |
| "start": 25.74, | |
| "end": 33.94 | |
| }, | |
| { | |
| "text": "Sample lyric line 4", | |
| "start": 33.94, | |
| "end": 42.29 | |
| }, | |
| { | |
| "text": "Sample lyric line 5", | |
| "start": 42.29, | |
| "end": 50.49 | |
| }, | |
| { | |
| "text": "Sample lyric line 6", | |
| "start": 50.49, | |
| "end": 58.95 | |
| }, | |
| { | |
| "text": "Sample lyric line 7", |
Comment on lines
+37
to
+41
| { | ||
| "text": "[Chris Martin]", | ||
| "start": 0, | ||
| "end": 6.186 | ||
| }, |
There was a problem hiding this comment.
This file commits full, real song lyrics into the repository (a complete lyric timeline). That can create IP/licensing concerns and also encourages committing generated artifacts. Consider removing from version control and keeping only small synthetic fixtures under data/ (or generating these files at runtime and adding them to .gitignore).
|
|
||
| const fadeWindow = Math.min(10, Math.max(1, Math.floor((end - start) / 2))); | ||
| const enterEnd = Math.min(end, start + fadeWindow); | ||
| const exitStart = Math.max(start, end - fadeWindow); |
There was a problem hiding this comment.
fadeWindow can force a 1-frame window, which makes the interpolate() inputRange non-monotonic when end === start + 1 (because enterEnd === end but exitStart === start). Remotion's interpolate expects a non-decreasing range and can throw at runtime for very short/invalid line timings. Ensure exitStart >= enterEnd (or special-case very short durations) before calling interpolate.
Suggested change
| const exitStart = Math.max(start, end - fadeWindow); | |
| const rawExitStart = Math.max(start, end - fadeWindow); | |
| const exitStart = Math.max(enterEnd, rawExitStart); |
|
|
||
| return textLines.map((text, index) => { | ||
| const start = index * slot; | ||
| const end = Math.max(start + MIN_LINE_DURATION_SEC, (index + 1) * slot); |
There was a problem hiding this comment.
When slot < MIN_LINE_DURATION_SEC, the end calculation can push end past totalDuration and create overlapping/out-of-bounds line windows (especially for the last lines). Consider capping end to totalDuration and/or not enforcing MIN_LINE_DURATION_SEC when the per-line slot is smaller than the minimum.
Suggested change
| const end = Math.max(start + MIN_LINE_DURATION_SEC, (index + 1) * slot); | |
| const slotEnd = (index + 1) * slot; | |
| const minEnd = start + MIN_LINE_DURATION_SEC; | |
| const end = Math.min( | |
| totalDuration, | |
| slot >= MIN_LINE_DURATION_SEC ? Math.max(minEnd, slotEnd) : slotEnd, | |
| ); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
.github/workflows/test-render.yml (1)
3-4: Consider addingpull_requesttrigger for continuous validation.
With manual-only dispatch, rendering regressions can merge undetected. Keepingworkflow_dispatchpluspull_requestgives faster feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-render.yml around lines 3 - 4, Update the GitHub Actions triggers to include pull_request in addition to workflow_dispatch so the workflow runs on PRs for continuous validation: modify the top-level "on:" block to list both "workflow_dispatch" and "pull_request" (keeping existing workflow_dispatch intact) so the workflow triggers on PR events as well as manual dispatch.scripts/render-test-video.js (2)
11-14: Add validation for input JSON structure.
readInputPropsparses JSON without validating that it contains the expectedlinesarray. If the JSON is malformed or missing required fields, the error will surface later during composition rendering with a less clear message.♻️ Proposed validation
const readInputProps = async () => { const contents = await fs.readFile(INPUT_JSON, 'utf-8'); - return JSON.parse(contents); + const parsed = JSON.parse(contents); + if (!parsed || !Array.isArray(parsed.lines)) { + throw new Error('Input JSON must contain a "lines" array.'); + } + return parsed; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/render-test-video.js` around lines 11 - 14, readInputProps currently reads and JSON.parses INPUT_JSON but does not validate the structure; update readInputProps to verify the parsed object contains a non-empty array at the expected property (e.g., parsed.lines) and throw a clear, early error if missing or invalid (include INPUT_JSON in the error text); also consider validating element types inside lines (strings/objects as expected) before returning so downstream functions that use readInputProps get a validated shape.
8-9: Consider making input/output paths configurable.The paths are hardcoded. For flexibility (e.g., running with different fixtures or output locations), consider accepting CLI arguments or environment variables similar to
phase3-fetch-song-lyrics.jsandphase4-generate-direction.js.♻️ Example enhancement
+const INPUT_JSON = process.env.RENDER_INPUT_JSON + ? path.join(PROJECT_ROOT, process.env.RENDER_INPUT_JSON) + : path.join(PROJECT_ROOT, 'data/test-lyrics.json'); +const OUTPUT_VIDEO = process.env.RENDER_OUTPUT_VIDEO + ? path.join(PROJECT_ROOT, process.env.RENDER_OUTPUT_VIDEO) + : path.join(PROJECT_ROOT, 'output/test-video.mp4'); -const INPUT_JSON = path.join(PROJECT_ROOT, 'data/test-lyrics.json'); -const OUTPUT_VIDEO = path.join(PROJECT_ROOT, 'output/test-video.mp4');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/render-test-video.js` around lines 8 - 9, Make INPUT_JSON and OUTPUT_VIDEO configurable instead of hardcoding: update the script to read input/output paths from CLI args (e.g., process.argv or a parsing lib) or environment variables (e.g., process.env.INPUT_JSON / process.env.OUTPUT_VIDEO) with the current constants INPUT_JSON and OUTPUT_VIDEO as fallbacks; modify the code that currently uses INPUT_JSON and OUTPUT_VIDEO so it uses the resolved values and add brief usage/help text when no args provided, following the same pattern used in phase3-fetch-song-lyrics.js / phase4-generate-direction.js.scripts/lib/lyrics-client.js (3)
46-51: Scoring can double-count exact matches.When
title === targetTitle, both the exact match (+80) and includes check (+30) will pass, resulting in a score of 110. This is benign since exact matches should rank highest anyway, but may be unintentional.♻️ Use else-if for mutually exclusive scoring
let score = 0; if (title === targetTitle) { score += 80; - } - if (title.includes(targetTitle)) { + } else if (title.includes(targetTitle)) { score += 30; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/lyrics-client.js` around lines 46 - 51, The current scoring logic double-counts exact matches because when title === targetTitle both the exact-match branch that adds 80 and the includes branch that adds 30 run; update the conditional in the function that computes score so the includes check is mutually exclusive (e.g., change the second if to else if or otherwise guard it with a negative exact-match check) so an exact match only gains the +80 from title === targetTitle and not the additional +30 from title.includes(targetTitle).
14-30: Consider logging suppressed fetch errors for debugging.The
fetchJsonhelper returnsnullfor all errors including network issues and timeouts. For production troubleshooting, logging these errors at a debug level could help diagnose connectivity problems.♻️ Add debug logging
} catch (error) { + if (process.env.DEBUG) { + console.debug(`[lyrics-client] Fetch failed for ${url}:`, error.message); + } return null; } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/lyrics-client.js` around lines 14 - 30, fetchJson currently swallows all fetch errors and timeouts by returning null with no record; update the catch block in fetchJson to log the caught error (including the request URL and REQUEST_TIMEOUT_MS) at a debug level before returning null so troubleshooting is possible—use your project's logger (or console.debug if none) and include error.message/stack and the URL parameter, leaving the return behavior unchanged; keep the AbortController usage (controller.signal) and timeout clearing logic as-is.
139-155: Nested loops may result in multiple API requests.With 2 title candidates × 2 artist candidates, this could make up to 4 sequential requests to LRCLib before trying Lyrics.ovh. For slow or rate-limited APIs, consider adding early exit logging or parallelizing candidates where safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/lyrics-client.js` around lines 139 - 155, The nested for-loops over titleCandidates and artistCandidates cause sequential calls to getFromLrcLib and then getFromLyricsOvh, which can produce many slow requests; refactor the lookup logic (the loops that call getFromLrcLib and getFromLyricsOvh) to run candidate lookups in parallel and short-circuit on the first successful result—for example, build the pair combinations from titleCandidates and artistCandidates, map them to promises calling getFromLrcLib, use Promise.allSettled or Promise.any to await results and return the first non-null value, and if none succeed, do the same parallelized approach for getFromLyricsOvh; also add a single early-exit log when a match is found.package.json (1)
9-9: Consider renaming thetestscript or adding a lightweight test alternative.The
testscript runsrender:test, which bundles and renders a full video. This is computationally expensive and may not be suitable for typical CI test runs or quick validation. Consider either:
- Renaming to something like
test:renderortest:e2e- Adding a lighter smoke test that validates JSON fixtures without full video rendering
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 9, The current npm "test" script points to "render:test" which is too heavy for CI; change scripts so "render:test" is renamed to a more explicit name like "test:render" or "test:e2e", and add a new lightweight "test" script that runs a fast smoke/fixture validator (for example a small Node script or jest command that checks JSON fixtures). Update package.json scripts to include both "test" (quick validation) and "test:render" (full video render) and ensure any CI config calls the new lightweight "test" instead of the heavy render task.scripts/phase3-fetch-song-lyrics.js (1)
51-52: Guard against missing song name or artist.If
resolveSongMetadatareturns a song object with missingnameorartistfields, the log statement will printundefined. Consider adding fallbacks for logging.♻️ Proposed fix
const {song} = songResolution; - console.log(`[phase-3] Selected song: ${song.name} - ${song.artist}`); + console.log(`[phase-3] Selected song: ${song.name || 'Unknown'} - ${song.artist || 'Unknown'}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/phase3-fetch-song-lyrics.js` around lines 51 - 52, The console log can print undefined if songResolution or its song fields are missing; update the extraction and logging to guard against null/undefined by first using a safe access (e.g., let song = songResolution?.song || {}) and then log using fallbacks for song.name and song.artist (e.g., use '<unknown title>' and '<unknown artist>' when those fields are falsy) so the log never prints "undefined" and remains informative.remotion/LyricsVideo.jsx (1)
29-35: Consider handling overlapping lyric lines.If input data contains overlapping time ranges (e.g., line A ends at 5.0s and line B starts at 4.5s), multiple
<p>elements will render simultaneously. This may be intentional for certain effects, but if not expected, lines could visually collide.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@remotion/LyricsVideo.jsx` around lines 29 - 35, The current lines.map callback (using variables lines, start, end, frame, fps) will render all lines whose ranges include the current frame, causing overlaps; update rendering to pick a single active line instead—compute the active line once (e.g., find the line with start <= frame <= end and with the latest start time or highest priority) and only render that line's <p>; modify the logic around the lines.map (or replace it) to determine activeIndex/activeLine before returning JSX so overlapping ranges show one chosen line rather than multiple simultaneous <p> elements.scripts/lib/ai-director.js (2)
127-128: Truncate error message to avoid leaking sensitive information.The error message includes up to 240 characters of the response body, which could potentially contain sensitive information or confusing debug output. Consider truncating further for production use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/ai-director.js` around lines 127 - 128, The thrown Error currently embeds up to 240 chars of bodyText which can leak sensitive data; reduce the exposed payload by truncating to a much smaller safe length (e.g., 60–80 chars) and append an ellipsis, or replace the snippet with a generic placeholder like "[response truncated]" while keeping the full bodyText available only in debug logs; update the throw site that uses bodyText (the throw new Error(`AI provider returned non-JSON response: ${bodyText.slice(0, 240)}`) expression) to use the shorter slice or placeholder and ensure any detailed logging of bodyText is gated behind debug-level logging, not the thrown error message.
446-450: Repair prompt includes raw AI content - may hit token limits.If the initial response is large or malformed, passing the entire
firstResponse.contentin the repair prompt could exceed token limits or cause issues. Consider truncating the broken content.♻️ Truncate broken content in repair prompt
const repairPrompt = [ 'Repair the following content into strict valid JSON only.', 'Do not change meaning. Keep all keys from the target schema.', - `Broken content: ${firstResponse.content}`, + `Broken content: ${firstResponse.content.slice(0, 2000)}`, ].join('\n');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/ai-director.js` around lines 446 - 450, The repairPrompt currently embeds the full firstResponse.content which can exceed token limits; update the code that builds repairPrompt (look for the repairPrompt constant in scripts/lib/ai-director.js) to safely truncate firstResponse.content before interpolation (e.g., use a helper/truncate function or .slice to limit to a reasonable max length and append an indicator like "...[truncated]" so intent is clear), then use the truncated string when constructing repairPrompt to avoid oversized prompts.data/phase4-direction.json (1)
38-119: Collapse this fixture to scene ranges plus true per-line overrides.Every
sceneDirections[].lineIndicesblock here is a contiguous range, and everylineDirections[]entry just repeats its scene defaults. Sincescripts/lib/ai-director.js:278-346already supportslineStartIndex/lineEndIndex, andscripts/lib/ai-director.js:389-402can synthesize per-line defaults, checking in the fully expanded form only adds diff churn and drift risk.Also applies to: 120-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/phase4-direction.json` around lines 38 - 119, The fixture repeats contiguous per-line defaults; collapse each sceneDirections[].lineIndices into sceneDirections[].lineStartIndex and .lineEndIndex (use the existing contiguous ranges) and remove the expanded per-line entries in lineDirections that merely duplicate the scene defaults, relying on scripts/lib/ai-director.js support (lineStartIndex/lineEndIndex handling at ~278-346 and per-line synthesis at ~389-402) so the file stores scene ranges plus only true per-line overrides.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-render.yml:
- Around line 13-29: Replace mutable action tags with pinned commit SHAs: update
the uses entries for actions/checkout@v4, actions/setup-node@v4, and
actions/upload-artifact@v4 to their corresponding full 40-character commit SHAs;
for example replace the string in the uses field for actions/checkout,
actions/setup-node (the step that sets node-version: 20), and
actions/upload-artifact (the upload artifact step) with the specific commit SHA
for each repository so the workflow references immutable commits.
In `@data/phase3-lyrics-fallback.json`:
- Around line 36-252: The file contains full commercial song lyrics stored under
the "lines" array (e.g., entries like "Look at the stars" and other complete
lines), which creates copyright risk; remove or replace those exact lyric
entries and instead commit a sanitized fixture: keep the "lines" array shape but
replace "text" values with synthetic or placeholder strings (e.g., short generic
lines or "[LYRIC_LINE_n]"), or store references/IDs to fetch licensed lyrics at
runtime; ensure the JSON schema (objects with "text","start","end") remains
intact so consumers like any parser reading "lines" continue to work (look for
the "lines" array and sample entries such as "Look at the stars" to locate and
update).
In `@data/phase3-lyrics.json`:
- Around line 37-158: The file embeds full third-party lyrics inside the JSON
"lines" array (each object with "text", "start", "end"); remove the commercial
lyric entries and replace them with synthetic fixture data or brief placeholders
(e.g., a few short non‑copyright sample lines or "<LYRIC_PLACEHOLDER>") so tests
can run without licensed content, ensuring any consumers of the "lines" array
still get the same shape (objects with "text","start","end") and updating any
tests or fixtures that asserted on exact lyric strings to use the synthetic
placeholders or mocked values.
In `@remotion/Root.jsx`:
- Line 17: Root.jsx currently hardcodes durationInFrames={150}, which truncates
real lyric timelines; change it to compute duration dynamically from the input
timeline/song duration (e.g., derive from the loaded phase3-lyrics timeline or
song duration field) and multiply seconds by the composition FPS or compute the
last frame index from the timeline entries; update the prop passed to the
Remotion composition (durationInFrames) to use that computed value (referencing
the Root component and the durationInFrames prop) so the composition length
matches the input data instead of a fixed 150 frames.
In `@scripts/lib/jiosaavn-client.js`:
- Around line 8-9: The RETRIES_PER_ENDPOINT env variable is parsed and then
incorrectly used as total attempts which causes PHASE3_JIOSAAVN_RETRIES=2 to
result in only two total calls and 0/NaN to skip requests; update the parsing
for REQUEST_TIMEOUT_MS and RETRIES_PER_ENDPOINT to sanitize invalid values (use
Number.parseInt or Number, fallback to a sensible default like 2, and clamp to a
non-negative integer), and change the call-site logic (the code using
RETRIES_PER_ENDPOINT around lines 53-60) to treat RETRIES_PER_ENDPOINT as a
retry count and compute attempts = retries + 1 before looping/attempting
requests so zero retries still performs one attempt. Ensure you reference
REQUEST_TIMEOUT_MS and RETRIES_PER_ENDPOINT in the fix so consumers get
sanitized integers and the attempts calculation is correct.
- Around line 194-202: The returned candidates are using raw API order
(songs.slice) instead of the ranking that produced the winner; update the return
to take the top-ranked entries from the ranked array and expose their song
objects (e.g., use ranked.slice(0,5).map(r => r.song)) so candidates reflect the
same ordering used by scoreSongCandidate and the selected song, leaving endpoint
and selectionMethod unchanged.
- Around line 122-127: The current normalizeForScore function removes all
non-ASCII letters causing scripts like Devanagari to be stripped; update its
regexes to be Unicode-aware by using Unicode property escapes (e.g., allow \p{L}
and \p{N}) with the /u (and /g) flags so non-Latin letters and numbers are
preserved, and apply the same change to any related regexes used in
scoreSongCandidate and its dependents so scoring compares normalized
Unicode-aware strings instead of dropping non-ASCII characters.
---
Nitpick comments:
In @.github/workflows/test-render.yml:
- Around line 3-4: Update the GitHub Actions triggers to include pull_request in
addition to workflow_dispatch so the workflow runs on PRs for continuous
validation: modify the top-level "on:" block to list both "workflow_dispatch"
and "pull_request" (keeping existing workflow_dispatch intact) so the workflow
triggers on PR events as well as manual dispatch.
In `@data/phase4-direction.json`:
- Around line 38-119: The fixture repeats contiguous per-line defaults; collapse
each sceneDirections[].lineIndices into sceneDirections[].lineStartIndex and
.lineEndIndex (use the existing contiguous ranges) and remove the expanded
per-line entries in lineDirections that merely duplicate the scene defaults,
relying on scripts/lib/ai-director.js support (lineStartIndex/lineEndIndex
handling at ~278-346 and per-line synthesis at ~389-402) so the file stores
scene ranges plus only true per-line overrides.
In `@package.json`:
- Line 9: The current npm "test" script points to "render:test" which is too
heavy for CI; change scripts so "render:test" is renamed to a more explicit name
like "test:render" or "test:e2e", and add a new lightweight "test" script that
runs a fast smoke/fixture validator (for example a small Node script or jest
command that checks JSON fixtures). Update package.json scripts to include both
"test" (quick validation) and "test:render" (full video render) and ensure any
CI config calls the new lightweight "test" instead of the heavy render task.
In `@remotion/LyricsVideo.jsx`:
- Around line 29-35: The current lines.map callback (using variables lines,
start, end, frame, fps) will render all lines whose ranges include the current
frame, causing overlaps; update rendering to pick a single active line
instead—compute the active line once (e.g., find the line with start <= frame <=
end and with the latest start time or highest priority) and only render that
line's <p>; modify the logic around the lines.map (or replace it) to determine
activeIndex/activeLine before returning JSX so overlapping ranges show one
chosen line rather than multiple simultaneous <p> elements.
In `@scripts/lib/ai-director.js`:
- Around line 127-128: The thrown Error currently embeds up to 240 chars of
bodyText which can leak sensitive data; reduce the exposed payload by truncating
to a much smaller safe length (e.g., 60–80 chars) and append an ellipsis, or
replace the snippet with a generic placeholder like "[response truncated]" while
keeping the full bodyText available only in debug logs; update the throw site
that uses bodyText (the throw new Error(`AI provider returned non-JSON response:
${bodyText.slice(0, 240)}`) expression) to use the shorter slice or placeholder
and ensure any detailed logging of bodyText is gated behind debug-level logging,
not the thrown error message.
- Around line 446-450: The repairPrompt currently embeds the full
firstResponse.content which can exceed token limits; update the code that builds
repairPrompt (look for the repairPrompt constant in scripts/lib/ai-director.js)
to safely truncate firstResponse.content before interpolation (e.g., use a
helper/truncate function or .slice to limit to a reasonable max length and
append an indicator like "...[truncated]" so intent is clear), then use the
truncated string when constructing repairPrompt to avoid oversized prompts.
In `@scripts/lib/lyrics-client.js`:
- Around line 46-51: The current scoring logic double-counts exact matches
because when title === targetTitle both the exact-match branch that adds 80 and
the includes branch that adds 30 run; update the conditional in the function
that computes score so the includes check is mutually exclusive (e.g., change
the second if to else if or otherwise guard it with a negative exact-match
check) so an exact match only gains the +80 from title === targetTitle and not
the additional +30 from title.includes(targetTitle).
- Around line 14-30: fetchJson currently swallows all fetch errors and timeouts
by returning null with no record; update the catch block in fetchJson to log the
caught error (including the request URL and REQUEST_TIMEOUT_MS) at a debug level
before returning null so troubleshooting is possible—use your project's logger
(or console.debug if none) and include error.message/stack and the URL
parameter, leaving the return behavior unchanged; keep the AbortController usage
(controller.signal) and timeout clearing logic as-is.
- Around line 139-155: The nested for-loops over titleCandidates and
artistCandidates cause sequential calls to getFromLrcLib and then
getFromLyricsOvh, which can produce many slow requests; refactor the lookup
logic (the loops that call getFromLrcLib and getFromLyricsOvh) to run candidate
lookups in parallel and short-circuit on the first successful result—for
example, build the pair combinations from titleCandidates and artistCandidates,
map them to promises calling getFromLrcLib, use Promise.allSettled or
Promise.any to await results and return the first non-null value, and if none
succeed, do the same parallelized approach for getFromLyricsOvh; also add a
single early-exit log when a match is found.
In `@scripts/phase3-fetch-song-lyrics.js`:
- Around line 51-52: The console log can print undefined if songResolution or
its song fields are missing; update the extraction and logging to guard against
null/undefined by first using a safe access (e.g., let song =
songResolution?.song || {}) and then log using fallbacks for song.name and
song.artist (e.g., use '<unknown title>' and '<unknown artist>' when those
fields are falsy) so the log never prints "undefined" and remains informative.
In `@scripts/render-test-video.js`:
- Around line 11-14: readInputProps currently reads and JSON.parses INPUT_JSON
but does not validate the structure; update readInputProps to verify the parsed
object contains a non-empty array at the expected property (e.g., parsed.lines)
and throw a clear, early error if missing or invalid (include INPUT_JSON in the
error text); also consider validating element types inside lines
(strings/objects as expected) before returning so downstream functions that use
readInputProps get a validated shape.
- Around line 8-9: Make INPUT_JSON and OUTPUT_VIDEO configurable instead of
hardcoding: update the script to read input/output paths from CLI args (e.g.,
process.argv or a parsing lib) or environment variables (e.g.,
process.env.INPUT_JSON / process.env.OUTPUT_VIDEO) with the current constants
INPUT_JSON and OUTPUT_VIDEO as fallbacks; modify the code that currently uses
INPUT_JSON and OUTPUT_VIDEO so it uses the resolved values and add brief
usage/help text when no args provided, following the same pattern used in
phase3-fetch-song-lyrics.js / phase4-generate-direction.js.
🪄 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: 29e62cac-eecc-4a2a-af7d-e43e86a7de3d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.env.example.github/workflows/test-render.yml.gitignoredata/phase3-lyrics-fallback.jsondata/phase3-lyrics.jsondata/phase4-direction.jsondata/test-lyrics.jsonoutput/.gitkeeppackage.jsonremotion/LyricsVideo.jsxremotion/Root.jsxremotion/index.jsxscripts/lib/ai-director.jsscripts/lib/jiosaavn-client.jsscripts/lib/lyrics-client.jsscripts/lib/lyrics-transform.jsscripts/phase3-fetch-song-lyrics.jsscripts/phase4-generate-direction.jsscripts/render-test-video.js
Comment on lines
+13
to
+29
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 | ||
| cache: npm | ||
|
|
||
| - name: Install dependencies | ||
| run: npm ci | ||
|
|
||
| - name: Render test video | ||
| run: npm run render:test | ||
|
|
||
| - name: Upload rendered video artifact | ||
| uses: actions/upload-artifact@v4 | ||
| with: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify workflow action refs that are NOT pinned to a 40-char commit SHA.
rg -nP '^\s*uses:\s*[^@]+@(?![0-9a-f]{40}\s*$).+' .github/workflowsRepository: Dcode9/MPLayer
Length of output: 279
Pin GitHub Actions to immutable commit SHAs instead of mutable version tags.
Lines 13, 16, and 28 use mutable tags (@v4) for actions/checkout, actions/setup-node, and actions/upload-artifact. This exposes the workflow to supply chain attacks if upstream actions are compromised. Replace with full commit SHAs (40-character format).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test-render.yml around lines 13 - 29, Replace mutable
action tags with pinned commit SHAs: update the uses entries for
actions/checkout@v4, actions/setup-node@v4, and actions/upload-artifact@v4 to
their corresponding full 40-character commit SHAs; for example replace the
string in the uses field for actions/checkout, actions/setup-node (the step that
sets node-version: 20), and actions/upload-artifact (the upload artifact step)
with the specific commit SHA for each repository so the workflow references
immutable commits.
Comment on lines
+36
to
+252
| "lines": [ | ||
| { | ||
| "text": "[Chris Martin]", | ||
| "start": 0, | ||
| "end": 6.186 | ||
| }, | ||
| { | ||
| "text": "Look at the stars", | ||
| "start": 6.186, | ||
| "end": 12.372 | ||
| }, | ||
| { | ||
| "text": "Look how they shine for you", | ||
| "start": 12.372, | ||
| "end": 18.558 | ||
| }, | ||
| { | ||
| "text": "And everything you do", | ||
| "start": 18.558, | ||
| "end": 24.744 | ||
| }, | ||
| { | ||
| "text": "Yeah, they were all yellow", | ||
| "start": 24.744, | ||
| "end": 30.93 | ||
| }, | ||
| { | ||
| "text": "I came along", | ||
| "start": 30.93, | ||
| "end": 37.116 | ||
| }, | ||
| { | ||
| "text": "I wrote a song for you", | ||
| "start": 37.116, | ||
| "end": 43.302 | ||
| }, | ||
| { | ||
| "text": "And all the things you do", | ||
| "start": 43.302, | ||
| "end": 49.488 | ||
| }, | ||
| { | ||
| "text": "And it was called \"Yellow\"", | ||
| "start": 49.488, | ||
| "end": 55.674 | ||
| }, | ||
| { | ||
| "text": "So then I took my turn", | ||
| "start": 55.674, | ||
| "end": 61.86 | ||
| }, | ||
| { | ||
| "text": "Oh, what a thing to have done", | ||
| "start": 61.86, | ||
| "end": 68.047 | ||
| }, | ||
| { | ||
| "text": "And it was all yellow", | ||
| "start": 68.047, | ||
| "end": 74.233 | ||
| }, | ||
| { | ||
| "text": "[Chris, Jonny & Will]", | ||
| "start": 74.233, | ||
| "end": 80.419 | ||
| }, | ||
| { | ||
| "text": "(Aah) Your skin, oh yeah, your skin and bones", | ||
| "start": 80.419, | ||
| "end": 86.605 | ||
| }, | ||
| { | ||
| "text": "(Ooh) Turn into something beautiful", | ||
| "start": 86.605, | ||
| "end": 92.791 | ||
| }, | ||
| { | ||
| "text": "(Aah) You know, you know I love you so", | ||
| "start": 92.791, | ||
| "end": 98.977 | ||
| }, | ||
| { | ||
| "text": "You know I love you so", | ||
| "start": 98.977, | ||
| "end": 105.163 | ||
| }, | ||
| { | ||
| "text": "[Chris Martin]", | ||
| "start": 105.163, | ||
| "end": 111.349 | ||
| }, | ||
| { | ||
| "text": "I swam across", | ||
| "start": 111.349, | ||
| "end": 117.535 | ||
| }, | ||
| { | ||
| "text": "I jumped across for you", | ||
| "start": 117.535, | ||
| "end": 123.721 | ||
| }, | ||
| { | ||
| "text": "Oh, what a thing to do", | ||
| "start": 123.721, | ||
| "end": 129.907 | ||
| }, | ||
| { | ||
| "text": "'Cause you were all yellow", | ||
| "start": 129.907, | ||
| "end": 136.093 | ||
| }, | ||
| { | ||
| "text": "I drew a line", | ||
| "start": 136.093, | ||
| "end": 142.279 | ||
| }, | ||
| { | ||
| "text": "I drew a line for you", | ||
| "start": 142.279, | ||
| "end": 148.465 | ||
| }, | ||
| { | ||
| "text": "Oh, what a thing to do", | ||
| "start": 148.465, | ||
| "end": 154.651 | ||
| }, | ||
| { | ||
| "text": "And it was all yellow", | ||
| "start": 154.651, | ||
| "end": 160.837 | ||
| }, | ||
| { | ||
| "text": "[Chris, Jonny & Will]", | ||
| "start": 160.837, | ||
| "end": 167.023 | ||
| }, | ||
| { | ||
| "text": "(Aah) Your skin, oh yeah, your skin and bones", | ||
| "start": 167.023, | ||
| "end": 173.209 | ||
| }, | ||
| { | ||
| "text": "(Ooh) Turn into something beautiful", | ||
| "start": 173.209, | ||
| "end": 179.395 | ||
| }, | ||
| { | ||
| "text": "(Aah) And you know", | ||
| "start": 179.395, | ||
| "end": 185.581 | ||
| }, | ||
| { | ||
| "text": "For you, I'd bleed myself dry", | ||
| "start": 185.581, | ||
| "end": 191.767 | ||
| }, | ||
| { | ||
| "text": "For you, I'd bleed myself dry", | ||
| "start": 191.767, | ||
| "end": 197.953 | ||
| }, | ||
| { | ||
| "text": "[ Chris Martin]", | ||
| "start": 197.953, | ||
| "end": 204.14 | ||
| }, | ||
| { | ||
| "text": "It's true, look how they shine for you", | ||
| "start": 204.14, | ||
| "end": 210.326 | ||
| }, | ||
| { | ||
| "text": "Look how they shine for you", | ||
| "start": 210.326, | ||
| "end": 216.512 | ||
| }, | ||
| { | ||
| "text": "Look how they shine for", | ||
| "start": 216.512, | ||
| "end": 222.698 | ||
| }, | ||
| { | ||
| "text": "Look how they shine for you", | ||
| "start": 222.698, | ||
| "end": 228.884 | ||
| }, | ||
| { | ||
| "text": "Look how they shine for you", | ||
| "start": 228.884, | ||
| "end": 235.07 | ||
| }, | ||
| { | ||
| "text": "Look how they shine", | ||
| "start": 235.07, | ||
| "end": 241.256 | ||
| }, | ||
| { | ||
| "text": "[ Chris Martin]", | ||
| "start": 241.256, | ||
| "end": 247.442 | ||
| }, | ||
| { | ||
| "text": "Look at the stars", | ||
| "start": 247.442, | ||
| "end": 253.628 | ||
| }, | ||
| { | ||
| "text": "Look how they shine for you", | ||
| "start": 253.628, | ||
| "end": 259.814 | ||
| }, | ||
| { | ||
| "text": "And all the things that you do", | ||
| "start": 259.814, | ||
| "end": 266 | ||
| } | ||
| ] |
There was a problem hiding this comment.
Avoid committing full commercial song lyrics in repository data files.
This segment stores complete third-party lyrics, which creates copyright/compliance risk for redistribution. Prefer sanitized fixtures (shape-only), short synthetic lines, or references fetched at runtime from licensed sources.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@data/phase3-lyrics-fallback.json` around lines 36 - 252, The file contains
full commercial song lyrics stored under the "lines" array (e.g., entries like
"Look at the stars" and other complete lines), which creates copyright risk;
remove or replace those exact lyric entries and instead commit a sanitized
fixture: keep the "lines" array shape but replace "text" values with synthetic
or placeholder strings (e.g., short generic lines or "[LYRIC_LINE_n]"), or store
references/IDs to fetch licensed lyrics at runtime; ensure the JSON schema
(objects with "text","start","end") remains intact so consumers like any parser
reading "lines" continue to work (look for the "lines" array and sample entries
such as "Look at the stars" to locate and update).
Comment on lines
+37
to
+158
| "lines": [ | ||
| { | ||
| "text": "थोड़ी फ़ुर्सत भी, मेरी जाँ, कभी बाँहों को दीजिए", | ||
| "start": 9.44, | ||
| "end": 17.59 | ||
| }, | ||
| { | ||
| "text": "थोड़ी फ़ुर्सत भी, मेरी जाँ, कभी बाँहों को दीजिए", | ||
| "start": 17.59, | ||
| "end": 25.74 | ||
| }, | ||
| { | ||
| "text": "आज की रात मज़ा हुस्न का आँखों से लीजिए", | ||
| "start": 25.74, | ||
| "end": 33.94 | ||
| }, | ||
| { | ||
| "text": "आज की रात मज़ा हुस्न का आँखों से लीजिए", | ||
| "start": 33.94, | ||
| "end": 42.29 | ||
| }, | ||
| { | ||
| "text": "वक़्त बर्बाद ना बिन बात की बातों में कीजिए", | ||
| "start": 42.29, | ||
| "end": 50.49 | ||
| }, | ||
| { | ||
| "text": "वक़्त बर्बाद ना बिन बात की बातों में कीजिए", | ||
| "start": 50.49, | ||
| "end": 58.95 | ||
| }, | ||
| { | ||
| "text": "आज की रात मज़ा हुस्न का आँखों से लीजिए", | ||
| "start": 58.95, | ||
| "end": 67.14 | ||
| }, | ||
| { | ||
| "text": "आज की रात मज़ा हुस्न का आँखों से लीजिए", | ||
| "start": 67.14, | ||
| "end": 75.91 | ||
| }, | ||
| { | ||
| "text": "ओ, जान की क़ुर्बानी ले-ले, दिलबर-जानी", | ||
| "start": 75.91, | ||
| "end": 81.04 | ||
| }, | ||
| { | ||
| "text": "तबाही पक्की है, आग तू, मैं पानी", | ||
| "start": 81.04, | ||
| "end": 85.16 | ||
| }, | ||
| { | ||
| "text": "जान की क़ुर्बानी ले-ले, दिलबर-जानी", | ||
| "start": 85.16, | ||
| "end": 89.35 | ||
| }, | ||
| { | ||
| "text": "तबाही पक्की है, आग तू, मैं पानी", | ||
| "start": 89.35, | ||
| "end": 110.65 | ||
| }, | ||
| { | ||
| "text": "मेरे महबूब, समझिए ज़रा मौक़े की नज़ाकत", | ||
| "start": 110.65, | ||
| "end": 120.9 | ||
| }, | ||
| { | ||
| "text": "आ, मेरे महबूब, समझिए ज़रा मौक़े की नज़ाकत", | ||
| "start": 120.9, | ||
| "end": 131.36 | ||
| }, | ||
| { | ||
| "text": "के ख़रीदी नहीं जा सकती हसीनों की इजाज़त", | ||
| "start": 131.36, | ||
| "end": 139.6 | ||
| }, | ||
| { | ||
| "text": "के ख़रीदी नहीं जा सकती हसीनों की इजाज़त", | ||
| "start": 139.6, | ||
| "end": 147.79 | ||
| }, | ||
| { | ||
| "text": "नाज़ इतना..., मेरी जाँ", | ||
| "start": 147.79, | ||
| "end": 156.16 | ||
| }, | ||
| { | ||
| "text": "नाज़ इतना भी नहीं खोखले वादों पे कीजिए", | ||
| "start": 156.16, | ||
| "end": 166.48 | ||
| }, | ||
| { | ||
| "text": "आज की रात मज़ा हुस्न का आँखों से लीजिए", | ||
| "start": 166.48, | ||
| "end": 174.77 | ||
| }, | ||
| { | ||
| "text": "आज की रात मज़ा हुस्न का आँखों से लीजिए", | ||
| "start": 174.77, | ||
| "end": 183.52 | ||
| }, | ||
| { | ||
| "text": "ओ, जान की क़ुर्बानी ले-ले, दिलबर-जानी", | ||
| "start": 183.52, | ||
| "end": 188.42 | ||
| }, | ||
| { | ||
| "text": "तबाही पक्की है, आग तू, मैं पानी", | ||
| "start": 188.42, | ||
| "end": 192.77 | ||
| }, | ||
| { | ||
| "text": "जान की क़ुर्बानी ले-ले, दिलबर-जानी", | ||
| "start": 192.77, | ||
| "end": 196.76 | ||
| }, | ||
| { | ||
| "text": "तबाही पक्की है, आग तू, मैं पानी", | ||
| "start": 196.76, | ||
| "end": 228 | ||
| } | ||
| ] |
There was a problem hiding this comment.
Do not version full third-party lyrics in repository fixtures.
This file embeds full commercial lyrics text, which is a licensing/compliance risk. Use synthetic fixtures for development and keep licensed content out of Git history.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@data/phase3-lyrics.json` around lines 37 - 158, The file embeds full
third-party lyrics inside the JSON "lines" array (each object with "text",
"start", "end"); remove the commercial lyric entries and replace them with
synthetic fixture data or brief placeholders (e.g., a few short non‑copyright
sample lines or "<LYRIC_PLACEHOLDER>") so tests can run without licensed
content, ensuring any consumers of the "lines" array still get the same shape
(objects with "text","start","end") and updating any tests or fixtures that
asserted on exact lyric strings to use the synthetic placeholders or mocked
values.
Comment on lines
+8
to
+9
| const REQUEST_TIMEOUT_MS = Number(process.env.PHASE3_HTTP_TIMEOUT_MS || 12000); | ||
| const RETRIES_PER_ENDPOINT = Number(process.env.PHASE3_JIOSAAVN_RETRIES || 2); |
There was a problem hiding this comment.
Treat PHASE3_JIOSAAVN_RETRIES as retries, and sanitize invalid env values.
Line 9 is named as a retry count, but Line 53 uses it as total attempts. That means PHASE3_JIOSAAVN_RETRIES=2 gives only two calls total, while 0 or NaN skips every request entirely.
Possible fix
-const REQUEST_TIMEOUT_MS = Number(process.env.PHASE3_HTTP_TIMEOUT_MS || 12000);
-const RETRIES_PER_ENDPOINT = Number(process.env.PHASE3_JIOSAAVN_RETRIES || 2);
+const parseNonNegativeInt = (value, fallback) => {
+ const parsed = Number.parseInt(String(value ?? ''), 10);
+ return Number.isFinite(parsed) && parsed >= 0 ? parsed : fallback;
+};
+
+const REQUEST_TIMEOUT_MS = Math.max(
+ 1,
+ parseNonNegativeInt(process.env.PHASE3_HTTP_TIMEOUT_MS, 12000)
+);
+const RETRIES_PER_ENDPOINT = parseNonNegativeInt(process.env.PHASE3_JIOSAAVN_RETRIES, 2);
...
- for (let attempt = 1; attempt <= RETRIES_PER_ENDPOINT; attempt += 1) {
+ const maxAttempts = RETRIES_PER_ENDPOINT + 1;
+ for (let attempt = 1; attempt <= maxAttempts; attempt += 1) {
try {
const data = await fetchJson(url);
return {data, endpoint: base};
} catch (error) {
errors.push(`${base} (attempt ${attempt}): ${error.message}`);
- if (attempt < RETRIES_PER_ENDPOINT) {
+ if (attempt < maxAttempts) {
await sleep(attempt * 400);
}
}
}Also applies to: 53-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lib/jiosaavn-client.js` around lines 8 - 9, The RETRIES_PER_ENDPOINT
env variable is parsed and then incorrectly used as total attempts which causes
PHASE3_JIOSAAVN_RETRIES=2 to result in only two total calls and 0/NaN to skip
requests; update the parsing for REQUEST_TIMEOUT_MS and RETRIES_PER_ENDPOINT to
sanitize invalid values (use Number.parseInt or Number, fallback to a sensible
default like 2, and clamp to a non-negative integer), and change the call-site
logic (the code using RETRIES_PER_ENDPOINT around lines 53-60) to treat
RETRIES_PER_ENDPOINT as a retry count and compute attempts = retries + 1 before
looping/attempting requests so zero retries still performs one attempt. Ensure
you reference REQUEST_TIMEOUT_MS and RETRIES_PER_ENDPOINT in the fix so
consumers get sanitized integers and the attempts calculation is correct.
Comment on lines
+122
to
+127
| const normalizeForScore = (value) => | ||
| String(value || '') | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9\s]/g, ' ') | ||
| .replace(/\s+/g, ' ') | ||
| .trim(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Do the Node.js versions this project targets support JavaScript RegExp Unicode property escapes such as \p{L}, \p{N}, and \p{M} with the /u flag?
💡 Result:
Yes, all currently supported Node.js versions (v20.x, v22.x, v24.x, v25.x as of 2026-04-14) fully support JavaScript RegExp Unicode property escapes such as \p{L}, \p{N}, and \p{M} when used with the /u flag. These escapes were introduced in Node.js v10.0.0 (April 2018), which shipped with V8 6.4 where the feature was enabled by default. Earlier versions (v6-v9) required runtime flags like --harmony_regexp_property. Node.js v20 and later are LTS or Current releases still receiving support until at least 2026-2028. All versions since v10 parse and execute \p{...}/u correctly for General_Category properties like L (Letter), N (Number), and M (Mark).
Citations:
- 1: https://stackoverflow.com/questions/60584050/unicode-regex-pl-not-working-in-nodejs
- 2: nodejs/node@ce66b02
- 3: https://github.com/nodejs/release
- 4: https://nodejs.org/en/about/releases
- 5: doc: add RegExp Unicode Property Escapes to intl nodejs/node#19052
- 6: providing version for NodeJS introducing unicode property escapes in RegExp mdn/browser-compat-data#4424
🏁 Script executed:
# Find Node.js version configuration
find . -maxdepth 2 -type f \( -name ".nvmrc" -o -name "package.json" -o -name ".node-version" -o -name ".github" -o -name "engines" \) | head -20Repository: Dcode9/MPLayer
Length of output: 72
🏁 Script executed:
# Look at package.json to find Node.js version requirements
cat package.json | grep -A 5 -B 5 "engines\|node"Repository: Dcode9/MPLayer
Length of output: 458
🏁 Script executed:
# Read the actual code file to verify the issue
cat -n scripts/lib/jiosaavn-client.js | sed -n '120,155p'Repository: Dcode9/MPLayer
Length of output: 1051
🏁 Script executed:
# Check for CI/CD configuration files that might indicate Node.js version requirements
find . -type f \( -name "*.yml" -o -name "*.yaml" \) | head -20Repository: Dcode9/MPLayer
Length of output: 93
🏁 Script executed:
# Look for any GitHub workflows or CI files
find .github -type f 2>/dev/null | head -20Repository: Dcode9/MPLayer
Length of output: 91
🏁 Script executed:
# Test the current regex behavior with a simple Node.js script to confirm the issue
node <<'EOF'
const normalizeForScore = (value) =>
String(value || '')
.toLowerCase()
.replace(/[^a-z0-9\s]/g, ' ')
.replace(/\s+/g, ' ')
.trim();
// Test with the example from the review
const result = normalizeForScore('आज की रात');
console.log(`Input: "आज की रात"`);
console.log(`Output: "${result}"`);
console.log(`Output is empty: ${result === ''}`);
// Test with mixed content
const result2 = normalizeForScore('Aaj Ki Raat आज की रात');
console.log(`\nInput: "Aaj Ki Raat आज की रात"`);
console.log(`Output: "${result2}"`);
EOFRepository: Dcode9/MPLayer
Length of output: 163
🏁 Script executed:
# Check the GitHub workflow file to see what Node.js version is used
cat .github/workflows/test-render.ymlRepository: Dcode9/MPLayer
Length of output: 727
Use Unicode-aware normalization for search scoring.
Line 125 drops every non-ASCII letter, so queries like आज की रात normalize to an empty string. Once that happens, line 134 returns 0 for every candidate and selection falls back to upstream API order. The project targets Node.js 20 (per GitHub Actions workflow), which fully supports the proposed Unicode property escape solution.
Proposed fix
const normalizeForScore = (value) =>
String(value || '')
+ .normalize('NFKD')
+ .replace(/\p{M}/gu, '')
.toLowerCase()
- .replace(/[^a-z0-9\s]/g, ' ')
+ .replace(/[^\p{L}\p{N}\s]/gu, ' ')
.replace(/\s+/g, ' ')
.trim();Also applies to: lines 129-152 (scoreSongCandidate function and its dependents).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lib/jiosaavn-client.js` around lines 122 - 127, The current
normalizeForScore function removes all non-ASCII letters causing scripts like
Devanagari to be stripped; update its regexes to be Unicode-aware by using
Unicode property escapes (e.g., allow \p{L} and \p{N}) with the /u (and /g)
flags so non-Latin letters and numbers are preserved, and apply the same change
to any related regexes used in scoreSongCandidate and its dependents so scoring
compares normalized Unicode-aware strings instead of dropping non-ASCII
characters.
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
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.
…en animation direction
Root.jsxto define the main composition for the lyrics video using Remotion.index.jsxto register the root component for rendering.ai-director.jsto handle AI-based animation direction generation with OpenAI and GLM-5 models.jiosaavn-client.jsfor fetching song metadata from JioSaavn API.lyrics-client.jsto retrieve lyrics from multiple sources with fallback mechanisms.lyrics-transform.jsto build timelines from synced and plain lyrics.phase3-fetch-song-lyrics.jsto fetch song metadata and lyrics, generating a structured JSON output.phase4-generate-direction.jsto generate animation direction based on fetched lyrics.render-test-video.jsto render a test video using the generated lyrics and animation direction.Summary by CodeRabbit
New Features
Chores