⚡ Bolt: Optimize JSON-LD parsing in MovieScraper#74
⚡ Bolt: Optimize JSON-LD parsing in MovieScraper#74bartholomej wants to merge 1 commit intomasterfrom
Conversation
Parsed the JSON-LD data once in the MovieScraper service instead of parsing it separately for each helper function (getMovieYear, getMovieDuration). This avoids redundant `JSON.parse` calls and improves performance. - Updated `getMovieYear` and `getMovieDuration` helpers to accept object. - Updated `MovieScraper` to parse JSON-LD string once. - Updated tests to reflect signature changes. - Added benchmark showing ~49x speedup for the parsing logic. Co-authored-by: bartholomej <5861310+bartholomej@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThis PR refactors JSON-LD parsing across the movie service to optimize performance. Instead of parsing JSON-LD strings multiple times in different helper functions, the code now parses JSON-LD once at the service layer and passes the resulting parsed object to helper functions, avoiding redundant parsing operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #74 +/- ##
==========================================
- Coverage 99.52% 99.36% -0.16%
==========================================
Files 28 28
Lines 629 632 +3
Branches 143 146 +3
==========================================
+ Hits 626 628 +2
- Misses 3 4 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🤖 Fix all issues with AI agents
In `@src/helpers/movie.helper.ts`:
- Around line 126-131: The function getMovieYear currently declares a return
type of number but returns null; update its signature to return number | null
and ensure the implementation consistently returns a number or null (e.g., keep
returning +jsonLd.dateCreated when present, otherwise null) so TypeScript strict
mode is satisfied; reference: function getMovieYear and the jsonLd.dateCreated
check.
- Around line 133-160: The function getMovieDuration currently declares a return
type of number but returns null in some paths and contains a typo "elelment";
update the function signature to return number | null and fix the typo "Get last
time elelment" → "Get last time element" (keep references to getMovieDuration
and parseISO8601Duration to locate the code). Ensure you only change the
declared return type and the typo string, leaving the logic unchanged.
🧹 Nitpick comments (3)
src/services/movie.service.ts (2)
44-53: Good optimization: single JSON.parse with proper error handling.The try/catch with a
nullfallback is appropriate. One consideration:
console.errorin a library can be noisy for consumers. Consider using a debug/verbose flag fromoptionsor simply swallowing silently (the helpers already handlenullgracefully).
62-62: Consider a lightweight interface instead ofanyforjsonLd.Using
anyloses all type safety. Since you're only accessingdateCreatedandduration, a minimal interface would catch typos at compile time and document the expected shape.💡 Suggested type
+interface JsonLdMovie { + dateCreated?: string; + duration?: string; + [key: string]: unknown; +} + private buildMovie( movieId: number, el: HTMLElement, asideEl: HTMLElement, pageClasses: string[], - jsonLd: any, + jsonLd: JsonLdMovie | null, options: CSFDOptions ): CSFDMovie {tests/movie.test.ts (1)
51-58: Test helper behavior diverges from production code when JSON-LD is missing.In the service (line 45-53 of
movie.service.ts), when no JSON-LD script tag exists,jsonLdstaysnull. Here, the?? '{}'fallback meansgetJsonLdreturns{}(an empty object) instead ofnull. While this currently produces the same downstream results, it means the tests don't exercise thenullpath that production code takes for movies without JSON-LD.💡 Suggested alignment with production behavior
const getJsonLd = (node: HTMLElement): any => { - const text = node.querySelector('script[type="application/ld+json"]')?.innerText ?? '{}'; - try { - return JSON.parse(text); - } catch (e) { - return null; - } + const text = node.querySelector('script[type="application/ld+json"]')?.innerText; + if (!text) return null; + try { + return JSON.parse(text); + } catch (e) { + return null; + } };
| export const getMovieYear = (jsonLd: any): number => { | ||
| if (jsonLd && jsonLd.dateCreated) { | ||
| return +jsonLd.dateCreated; | ||
| } catch (error) { | ||
| console.error('node-csfd-api: Error parsing JSON-LD', error); | ||
| return null; | ||
| } | ||
| return null; | ||
| }; |
There was a problem hiding this comment.
Return type should be number | null.
getMovieYear returns null on line 130 but the signature declares : number. This is misleading and will cause type errors in strict mode.
🐛 Proposed fix
-export const getMovieYear = (jsonLd: any): number => {
+export const getMovieYear = (jsonLd: any): number | null => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getMovieYear = (jsonLd: any): number => { | |
| if (jsonLd && jsonLd.dateCreated) { | |
| return +jsonLd.dateCreated; | |
| } catch (error) { | |
| console.error('node-csfd-api: Error parsing JSON-LD', error); | |
| return null; | |
| } | |
| return null; | |
| }; | |
| export const getMovieYear = (jsonLd: any): number | null => { | |
| if (jsonLd && jsonLd.dateCreated) { | |
| return +jsonLd.dateCreated; | |
| } | |
| return null; | |
| }; |
🤖 Prompt for AI Agents
In `@src/helpers/movie.helper.ts` around lines 126 - 131, The function
getMovieYear currently declares a return type of number but returns null; update
its signature to return number | null and ensure the implementation consistently
returns a number or null (e.g., keep returning +jsonLd.dateCreated when present,
otherwise null) so TypeScript strict mode is satisfied; reference: function
getMovieYear and the jsonLd.dateCreated check.
| export const getMovieDuration = (jsonLd: any, el: HTMLElement): number => { | ||
| let duration = null; | ||
| try { | ||
| const jsonLd = JSON.parse(jsonLdRaw); | ||
| duration = jsonLd.duration; | ||
| return parseISO8601Duration(duration); | ||
| } catch (error) { | ||
| const origin = el.querySelector('.origin').innerText; | ||
| const timeString = origin.split(','); | ||
| if (timeString.length > 2) { | ||
| // Get last time elelment | ||
| const timeString2 = timeString.pop().trim(); | ||
| // Clean it | ||
| const timeRaw = timeString2.split('(')[0].trim(); | ||
| // Split by minutes and hours | ||
| const hoursMinsRaw = timeRaw.split('min')[0]; | ||
| const hoursMins = hoursMinsRaw.split('h'); | ||
| // Resolve hours + minutes format | ||
| duration = hoursMins.length > 1 ? +hoursMins[0] * 60 + +hoursMins[1] : +hoursMins[0]; | ||
| return duration; | ||
| } else { | ||
| return null; | ||
| if (jsonLd && jsonLd.duration) { | ||
| duration = jsonLd.duration; | ||
| return parseISO8601Duration(duration); | ||
| } | ||
| } catch (error) { | ||
| // ignore | ||
| } | ||
|
|
||
| const origin = el.querySelector('.origin').innerText; | ||
| const timeString = origin.split(','); | ||
| if (timeString.length > 2) { | ||
| // Get last time elelment | ||
| const timeString2 = timeString.pop().trim(); | ||
| // Clean it | ||
| const timeRaw = timeString2.split('(')[0].trim(); | ||
| // Split by minutes and hours | ||
| const hoursMinsRaw = timeRaw.split('min')[0]; | ||
| const hoursMins = hoursMinsRaw.split('h'); | ||
| // Resolve hours + minutes format | ||
| duration = hoursMins.length > 1 ? +hoursMins[0] * 60 + +hoursMins[1] : +hoursMins[0]; | ||
| return duration; | ||
| } else { | ||
| return null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Return type should be number | null; minor typo on line 147.
Same issue as getMovieYear — the declared return type is : number but null is returned on line 158. Also, line 147 has a typo: "elelment" → "element".
🐛 Proposed fix
-export const getMovieDuration = (jsonLd: any, el: HTMLElement): number => {
+export const getMovieDuration = (jsonLd: any, el: HTMLElement): number | null => {
let duration = null;
try {
if (jsonLd && jsonLd.duration) {
duration = jsonLd.duration;
return parseISO8601Duration(duration);
}
} catch (error) {
// ignore
}
const origin = el.querySelector('.origin').innerText;
const timeString = origin.split(',');
if (timeString.length > 2) {
- // Get last time elelment
+ // Get last time element🤖 Prompt for AI Agents
In `@src/helpers/movie.helper.ts` around lines 133 - 160, The function
getMovieDuration currently declares a return type of number but returns null in
some paths and contains a typo "elelment"; update the function signature to
return number | null and fix the typo "Get last time elelment" → "Get last time
element" (keep references to getMovieDuration and parseISO8601Duration to locate
the code). Ensure you only change the declared return type and the typo string,
leaving the logic unchanged.
2 participants
💡 What: Optimized JSON-LD parsing in
MovieScraper. Instead of parsing the JSON-LD string inside each helper function (getMovieYear,getMovieDuration), the JSON is now parsed once in the service and the resulting object is passed to the helpers.🎯 Why: Redundant
JSON.parseoperations are expensive, especially for larger JSON objects found in movie details. This change eliminates the overhead of parsing the same string multiple times.📊 Impact:
JSON.parsecalls from O(N) (where N is number of fields extracted from JSON) to O(1).🔬 Measurement:
yarn testpasses (unit tests updated, live tests verify correctness).PR created automatically by Jules for task 15333492157311741926 started by @bartholomej
Summary by CodeRabbit
Performance Improvements
Documentation