⚡ Bolt: Optimize JSON-LD parsing in movie service#83
⚡ Bolt: Optimize JSON-LD parsing in movie service#83bartholomej wants to merge 1 commit intomasterfrom
Conversation
- Move JSON.parse call from helpers to service layer - Parse JSON-LD once per movie request instead of multiple times - Update helper signatures to accept parsed object - Update tests to reflect changes 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 handling by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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 #83 +/- ##
==========================================
- Coverage 99.52% 99.20% -0.32%
==========================================
Files 28 28
Lines 629 632 +3
Branches 143 145 +2
==========================================
+ Hits 626 627 +1
- Misses 3 5 +2 ☔ 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.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@src/helpers/movie.helper.ts`:
- Around line 148-153: getMovieYear is typed to return number but returns null;
update its signature to return number | null and do the same for
getMovieDuration (change their return types to number | null), verify any
callers handle null or adjust them accordingly, and ensure the exported function
declarations (getMovieYear and getMovieDuration) match the new union return
type.
In `@src/services/movie.service.ts`:
- Around line 46-52: The code currently reads the LD+JSON text with
movieHtml.querySelector('script[type="application/ld+json"]').innerText which
can throw if the element is missing; update the retrieval so it uses optional
chaining to safely access innerText (e.g., via querySelector(...)? .innerText)
and provide a safe default (empty string or null) before attempting JSON.parse
in the movieJsonLd parse block so buildMovie(...) receives a properly
null/parsed movieJsonLd; ensure you only call JSON.parse when the retrieved
jsonLd string is non-null/non-empty and keep the existing try/catch around the
parse.
In `@tests/movie.test.ts`:
- Around line 386-401: Rename the duplicate test titles to be unique so failures
are identifiable; update the two tests that call getMovieYear with movieJsonLd
and movieRichJsonLd to different names (e.g., change the test title for the
movieRichJsonLd case from 'Year' to 'Year Rich' or similar) while leaving the
other tests (including the seriesJsonLd and null case) unchanged; ensure only
the test name strings are modified around the test(...) calls that reference
getMovieYear, movieJsonLd, and movieRichJsonLd.
🧹 Nitpick comments (3)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@src/helpers/movie.helper.ts`: - Around line 148-153: getMovieYear is typed to return number but returns null; update its signature to return number | null and do the same for getMovieDuration (change their return types to number | null), verify any callers handle null or adjust them accordingly, and ensure the exported function declarations (getMovieYear and getMovieDuration) match the new union return type. In `@src/services/movie.service.ts`: - Around line 46-52: The code currently reads the LD+JSON text with movieHtml.querySelector('script[type="application/ld+json"]').innerText which can throw if the element is missing; update the retrieval so it uses optional chaining to safely access innerText (e.g., via querySelector(...)? .innerText) and provide a safe default (empty string or null) before attempting JSON.parse in the movieJsonLd parse block so buildMovie(...) receives a properly null/parsed movieJsonLd; ensure you only call JSON.parse when the retrieved jsonLd string is non-null/non-empty and keep the existing try/catch around the parse. In `@tests/movie.test.ts`: - Around line 386-401: Rename the duplicate test titles to be unique so failures are identifiable; update the two tests that call getMovieYear with movieJsonLd and movieRichJsonLd to different names (e.g., change the test title for the movieRichJsonLd case from 'Year' to 'Year Rich' or similar) while leaving the other tests (including the seriesJsonLd and null case) unchanged; ensure only the test name strings are modified around the test(...) calls that reference getMovieYear, movieJsonLd, and movieRichJsonLd.src/helpers/movie.helper.ts (1)
148-153: Return type should includenull.
getMovieYearis declared as returningnumberbut returnsnullon line 152. Same issue applies togetMovieDuration(line 155). While this may have been pre-existing (and works if strict null checks are disabled), it's worth fixing since you're already updating these signatures.Proposed fix
-export const getMovieYear = (jsonLd: MovieJsonLd | null): number => { +export const getMovieYear = (jsonLd: MovieJsonLd | null): number | null => {-export const getMovieDuration = (jsonLd: MovieJsonLd | null, el: HTMLElement): number => { +export const getMovieDuration = (jsonLd: MovieJsonLd | null, el: HTMLElement): number | null => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helpers/movie.helper.ts` around lines 148 - 153, getMovieYear is typed to return number but returns null; update its signature to return number | null and do the same for getMovieDuration (change their return types to number | null), verify any callers handle null or adjust them accordingly, and ensure the exported function declarations (getMovieYear and getMovieDuration) match the new union return type.src/services/movie.service.ts (1)
46-52: Good: JSON-LD parsed once with proper error handling.The try/catch around
JSON.parsewith fallback tonullis appropriate. This centralizes parsing and avoids redundant work in helpers.One note: line 45 (
movieHtml.querySelector('script[type="application/ld+json"]').innerText) can throw if the element isn't found. While pre-existing, you could harden it with optional chaining since you're already touching this area:Optional hardening
- const jsonLd = movieHtml.querySelector('script[type="application/ld+json"]').innerText; - let movieJsonLd: MovieJsonLd | null = null; - try { - movieJsonLd = JSON.parse(jsonLd); - } catch (e) { - console.error('node-csfd-api: Error parsing JSON-LD', e); - } + const jsonLdText = movieHtml.querySelector('script[type="application/ld+json"]')?.innerText; + let movieJsonLd: MovieJsonLd | null = null; + if (jsonLdText) { + try { + movieJsonLd = JSON.parse(jsonLdText); + } catch (e) { + console.error('node-csfd-api: Error parsing JSON-LD', e); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/movie.service.ts` around lines 46 - 52, The code currently reads the LD+JSON text with movieHtml.querySelector('script[type="application/ld+json"]').innerText which can throw if the element is missing; update the retrieval so it uses optional chaining to safely access innerText (e.g., via querySelector(...)? .innerText) and provide a safe default (empty string or null) before attempting JSON.parse in the movieJsonLd parse block so buildMovie(...) receives a properly null/parsed movieJsonLd; ensure you only call JSON.parse when the retrieved jsonLd string is non-null/non-empty and keep the existing try/catch around the parse.tests/movie.test.ts (1)
386-401: Duplicate test names: both year tests are named'Year'.Lines 387 and 390 both use
test('Year', ...). This makes it harder to identify which test failed. Consider giving them distinct names (e.g.,'Year Rich').🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/movie.test.ts` around lines 386 - 401, Rename the duplicate test titles to be unique so failures are identifiable; update the two tests that call getMovieYear with movieJsonLd and movieRichJsonLd to different names (e.g., change the test title for the movieRichJsonLd case from 'Year' to 'Year Rich' or similar) while leaving the other tests (including the seriesJsonLd and null case) unchanged; ensure only the test name strings are modified around the test(...) calls that reference getMovieYear, movieJsonLd, and movieRichJsonLd.
bartholomej
deleted the
bolt/optimize-json-ld-parsing-1526776526782896992
branch
2 participants
JSON.parsefromgetMovieYearandgetMovieDurationhelpers into theMovieScraperservice.JSON.parseonly once per request instead of 2+ times.PR created automatically by Jules for task 1526776526782896992 started by @bartholomej
Summary by CodeRabbit
Bug Fixes
Refactor