Conversation
bartholomej
force-pushed
the
hybrid
branch
2 times, most recently
from
4d9756a to
32b39fd
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18 +/- ##
===========================================
+ Coverage 98.67% 100.00% +1.32%
===========================================
Files 25 25
Lines 530 535 +5
Branches 105 115 +10
===========================================
+ Hits 523 535 +12
+ Misses 7 0 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bartholomej
force-pushed
the
master
branch
2 times, most recently
from
0a3f327 to
12e71ec
Compare
bartholomej
force-pushed
the
hybrid
branch
2 times, most recently
from
3fe5896 to
e28bcdd
Compare
bartholomej
force-pushed
the
hybrid
branch
3 times, most recently
from
eba4339 to
5b8519c
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRepo-wide migration from interface modules to DTO modules, systematic renaming of helper APIs to context-prefixed variants (e.g., getId → getMovieId/getCreatorId), TypeScript and build-system changes (tsdown + copyAndFixPackageJson), and new example projects and tests updated for the new API surface. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Build as tsdown
participant Plugin as copyAndFixPackageJson
participant Src as src/
participant DTO as src/dto/
participant Helpers as src/helpers/
participant Services as src/services/
Dev->>Src: change imports & rename helpers
Dev->>Build: add tsdown.config.ts
Dev->>Plugin: add package-json-fix.rolldown.ts
Build->>Src: compile entries (esm + cjs + dts)
Build->>Plugin: closeBundle -> copy & sanitize package.json -> write dist/package.json
Services->>Helpers: call getMovieX / getCreatorX / getSearchX / getCinemaX / getUserRatingX
Helpers->>DTO: consume DTO types
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Areas to focus review:
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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 |
|
Caution No docstrings were generated. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
src/helpers/global.helper.ts (1)
61-71: ISO‑8601 duration parsing: null‑match crash and mandatory “T” bug
- iso.match can be null → getDuration(null) will throw.
- Your regex requires “T”; durations without a time part won’t match.
Also, returning only minutes ignores other units. Safer minimal fix below.
Apply:
export const parseISO8601Duration = (iso: string): number => { - const iso8601DurationRegex = - /(-)?P(?:([.,\d]+)Y)?(?:([.,\d]+)M)?(?:([.,\d]+)W)?(?:([.,\d]+)D)?T(?:([.,\d]+)H)?(?:([.,\d]+)M)?(?:([.,\d]+)S)?/; - - const matches = iso.match(iso8601DurationRegex); - - const duration = getDuration(matches); - - return +duration.minutes; + const iso8601DurationRegex = + /^(-)?P(?:([.,\d]+)Y)?(?:([.,\d]+)M)?(?:([.,\d]+)W)?(?:([.,\d]+)D)?(?:T(?:(?:([.,\d]+)H)?(?:([.,\d]+)M)?(?:([.,\d]+)S)?))?$/; + const m = iso.match(iso8601DurationRegex); + if (!m) return NaN; + const sign = m[1] ? -1 : 1; + const years = parseFloat(m[2] ?? '0'); + const months = parseFloat(m[3] ?? '0'); + const weeks = parseFloat(m[4] ?? '0'); + const days = parseFloat(m[5] ?? '0'); + const hours = parseFloat(m[6] ?? '0'); + const minutes = parseFloat(m[7] ?? '0'); + const seconds = parseFloat(m[8] ?? '0'); + // Approximate months/years in minutes (30d/month, 365d/year) + const total = + years * 525600 + months * 43200 + weeks * 10080 + days * 1440 + + hours * 60 + minutes + seconds / 60; + return sign * total; }src/services/user-ratings.service.ts (3)
34-35: pages calculation can throw when .pagination is missingYou use optional chaining on pagesNode, but index with pagesNode again, which will crash if it’s null.
Apply:
- const pages = +pagesNode?.childNodes[pagesNode.childNodes.length - 4].rawText || 1; + const pages = + pagesNode && pagesNode.childNodes.length >= 4 + ? +pagesNode.childNodes[pagesNode.childNodes.length - 4].rawText + : 1;
25-36: films array never reset; results can accumulate across callsInitialize this.films for each scrape to avoid data leakage if the instance is reused.
Apply:
public async userRatings( user: string | number, config?: CSFDUserRatingConfig ): Promise<CSFDUserRatings[]> { - let allMovies: CSFDUserRatings[] = []; + this.films = []; + let allMovies: CSFDUserRatings[] = [];
61-62: Parameter type mismatch: getPage accepts non‑optional configCall sites pass possibly undefined. Make config optional in getPage.
Apply:
- private getPage(config: CSFDUserRatingConfig, movies: HTMLElement[]) { + private getPage(config: CSFDUserRatingConfig | undefined, movies: HTMLElement[]) {tests/services.test.ts (1)
88-106:console.warnassertion never runs and no spy is set.The matcher isn’t called, and the call happens before any spy can observe it.
Apply this rewrite (also import
viat top):-import { describe, expect, test } from 'vitest'; +import { describe, expect, test, vi } from 'vitest'; @@ -describe('Exclude + includes together', () => { - // Fetch data with excludes + includes - const userRatingsScraper = new UserRatingsScraper(); - const resBoth: Promise<CSFDUserRatings[]> = userRatingsScraper.userRatings(USER, { - includesOnly: ['seriál'], - excludes: ['film'] - }); - - test('Should have warning', () => { - expect(console.warn).toHaveBeenCalled; - }); - - test('Should use includesOnly', async () => { - const results = await resBoth; - - const tvSeries = results.filter((item) => item.type === 'seriál'); - expect(tvSeries.length).toBe<number>(results.length); - }); -}); +describe('Exclude + includes together', () => { + test('Should have warning', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const userRatingsScraper = new UserRatingsScraper(); + await userRatingsScraper.userRatings(USER, { + includesOnly: ['seriál'], + excludes: ['film'] + }); + expect(warnSpy).toHaveBeenCalled(); + warnSpy.mockRestore(); + }); + + test('Should use includesOnly', async () => { + const userRatingsScraper = new UserRatingsScraper(); + const results = await userRatingsScraper.userRatings(USER, { + includesOnly: ['seriál'], + excludes: ['film'] + }); + const tvSeries = results.filter((item) => item.type === 'seriál'); + expect(tvSeries.length).toBe<number>(results.length); + }); +});tests/fetchers.test.ts (2)
88-96: 404 test doesn’t assert rejection correctly.Assert promise rejection instead of catching and using
toContain.Apply:
- test('Fetch error URL', async () => { - try { - const url = userRatingsUrl(badId); - const html = await fetchPage(url); - expect(html).toBe('Error'); - } catch (e) { - expect(e).toContain(Error); - } - }); + test('Fetch error URL', async () => { + await expect(fetchPage(userRatingsUrl(badId))).rejects.toThrow(); + });
100-107: Same issue as above for movie 404.Apply:
- test('Fetch error URL', async () => { - try { - const url = movieUrl(badId); - const html = await fetchPage(url); - expect(html).toBe('Error'); - } catch (e) { - expect(e).toThrow(Error); - } - }); + test('Fetch error URL', async () => { + await expect(fetchPage(movieUrl(badId))).rejects.toThrow(); + });src/services/creator.service.ts (1)
20-23: Guard against missing DOM nodes before parsingIf
.creator-aboutor.creator-filmographyis missing, helper calls will throw. Fail fast with a clear error.Apply this diff:
- const asideNode = creatorHtml.querySelector('.creator-about'); - const filmsNode = creatorHtml.querySelector('.creator-filmography'); - this.buildCreator(+creatorId, asideNode, filmsNode); + const asideNode = creatorHtml.querySelector('.creator-about'); + const filmsNode = creatorHtml.querySelector('.creator-filmography'); + if (!asideNode || !filmsNode) { + throw new Error('node-csfd-api: Unexpected HTML structure for creator page.'); + } + this.buildCreator(id, asideNode, filmsNode);src/services/movie.service.ts (1)
41-46: Guard null DOM nodes and JSON‑LD to prevent runtime crashesSeveral selectors are assumed present; any CSFD layout change or non-200 page will throw (e.g.,
.page-content,.aside-movie-profile,.main-movie-profile, JSON‑LD script). Add checks and safe fallbacks before callingbuildMovie.Apply:
- const pageClasses = movieHtml.querySelector('.page-content').classNames.split(' '); - const asideNode = movieHtml.querySelector('.aside-movie-profile'); - const movieNode = movieHtml.querySelector('.main-movie-profile'); - const jsonLd = movieHtml.querySelector('script[type="application/ld+json"]').innerText; - this.buildMovie(+movieId, movieNode, asideNode, pageClasses, jsonLd); + const pageEl = movieHtml.querySelector('.page-content'); + const asideNode = movieHtml.querySelector('.aside-movie-profile'); + const movieNode = movieHtml.querySelector('.main-movie-profile'); + const jsonLdEl = movieHtml.querySelector('script[type="application/ld+json"]'); + + if (!pageEl || !movieNode || !asideNode) { + throw new Error('node-csfd-api: Unexpected page structure'); + } + + const pageClasses = pageEl.classNames?.split(' ') ?? []; + const jsonLd = jsonLdEl?.innerText ?? '{}'; + this.buildMovie(+movieId, movieNode, asideNode, pageClasses, jsonLd);src/index.ts (1)
13-19: Constructor signature change is a breaking API for consumers instantiatingCsfd
searchServicewas inserted beforecinemaService. Anyone doingnew Csfd(...)will break. Either keep backward compatibility or switch to a params object.Option: support old 4‑arg form transparently.
-export class Csfd { - constructor( - private userRatingsService: UserRatingsScraper, - private movieService: MovieScraper, - private creatorService: CreatorScraper, - private searchService: SearchScraper, - private cinemaService: CinemaScraper - ) { } +export class Csfd { + constructor( + private userRatingsService: UserRatingsScraper, + private movieService: MovieScraper, + private creatorService: CreatorScraper, + searchOrCinema: SearchScraper | CinemaScraper, + cinemaMaybe?: CinemaScraper + ) { + if (searchOrCinema instanceof SearchScraper) { + this.searchService = searchOrCinema; + this.cinemaService = cinemaMaybe!; + } else { + this.cinemaService = searchOrCinema; + this.searchService = new SearchScraper(); + } + }Alternatively, deprecate direct constructor use and expose only the
csfdfactory.src/helpers/search.helper.ts (1)
49-55: Bug:Array.from(el && el.querySelectorAll(...))can throwIf
elis falsy,el && ...becomesfalseandArray.from(false)throws. Also avoid shadowing param in callback.- const peopleNode = Array.from(el && el.querySelectorAll('.article-content p')).find((el) => - el.textContent.includes(who) - ); + const peopleNode = Array.from(el?.querySelectorAll('.article-content p') ?? []).find((p) => + p.textContent?.includes(who) + );src/helpers/cinema.helper.ts (1)
27-45: Harden coordinate parsing; avoid brittle split('q=')Current code throws if href is missing or query param is structured differently. Use URLSearchParams and add guards.
export const getCinemaCoords = (el: HTMLElement | null): { lat: number; lng: number } | null => { if (!el) return null; const linkMapsEl = el.querySelector('a[href*="q="]'); if (!linkMapsEl) return null; - const linkMaps = linkMapsEl.getAttribute('href'); - const [_, latLng] = linkMaps.split('q='); - - const coords = latLng.split(','); + const href = linkMapsEl.getAttribute('href') || ''; + const qs = href.split('?')[1] ?? ''; + const params = new URLSearchParams(qs); + const q = params.get('q') || ''; + if (!q) return null; + const coords = q.split(','); if (coords.length !== 2) return null; const lat = Number(coords[0]); const lng = Number(coords[1]); if (Number.isFinite(lat) && Number.isFinite(lng)) { return { lat, lng }; } return null; };src/helpers/creator.helper.ts (1)
22-43: Fix types and regex; age can be nullAge is set to
nullbut typed asnumber. Also,nar.should escape the dot.-export const getCreatorBirthdayInfo = ( - el: HTMLElement | null -): { birthday: string; age: number; birthPlace: string } => { +export const getCreatorBirthdayInfo = ( + el: HTMLElement | null +): { birthday: string; age: number | null; birthPlace: string } => { + if (!el) return { birthday: '', age: null, birthPlace: '' }; const infoBlock = el.querySelector('h1 + p'); const text = infoBlock?.innerHTML.trim(); const birthPlaceRow = infoBlock?.querySelector('.info-place')?.innerHTML.trim(); const ageRow = infoBlock?.querySelector('.info')?.innerHTML.trim(); let birthday: string = ''; if (text) { const parts = text.split('\n'); const birthdayRow = parts.find((x) => x.includes('nar.')); birthday = birthdayRow ? parseBirthday(birthdayRow) : ''; } - const age = ageRow ? +parseAge(ageRow) : null; + const age = ageRow ? +parseAge(ageRow) : null; const birthPlace = birthPlaceRow ? parseBirthPlace(birthPlaceRow) : ''; return { birthday, age, birthPlace }; };And update helper types below:
-const parseBirthday = (text: string): any => { - return text.replace(/nar./g, '').trim(); +const parseBirthday = (text: string): string => { + return text.replace(/nar\./g, '').trim(); }; -const parseAge = (text: string): any => { +const parseAge = (text: string): string => { return text.trim().replace(/\(/g, '').replace(/let\)/g, '').trim(); }; -const parseBirthPlace = (text: string): any => { +const parseBirthPlace = (text: string): string => { return text.trim().replace(/<br>/g, '').trim(); };
🧹 Nitpick comments (44)
src/helpers/global.helper.ts (1)
4-12: Return type can be null; align the signatureparseIdFromUrl returns null when it can’t parse. Either make it number | null or return NaN/0 consistently.
Apply:
-export const parseIdFromUrl = (url: string): number => { +export const parseIdFromUrl = (url: string): number | null => {tsconfig.json (1)
10-10: strict vs strictNullChecks mismatchYou’ve enabled strict but disabled strictNullChecks. Acceptable short‑term, but it masks real nullability issues seen in helpers. Plan to enable soon.
src/services/user-ratings.service.ts (2)
51-53: Optional chaining for allPagesDelayInside the allPages branch, use optional chaining for clarity and to satisfy strict nullability if enabled later.
Apply:
- if (config.allPagesDelay) { + if (config?.allPagesDelay) { await sleep(config.allPagesDelay); }
95-104: Redundant cast and double type parsingcolorRating helper already returns CSFDColorRating; drop the cast. Also, you compute type earlier during filtering—consider threading it down to avoid recomputation.
Apply:
- colorRating: getUserRatingColorRating(el) as CSFDColorRating, + colorRating: getUserRatingColorRating(el),src/helpers/user-ratings.helper.ts (4)
6-9: Nullability mismatch on IDparseIdFromUrl can return null; reflect that in the signature to prevent latent NPEs.
Apply:
-export const getUserRatingId = (el: HTMLElement): number => { +export const getUserRatingId = (el: HTMLElement): number | null => {
28-30: Nullability mismatch on yearYear extraction can fail; update the return type accordingly.
Apply:
-export const getUserRatingYear = (el: HTMLElement): number => { +export const getUserRatingYear = (el: HTMLElement): number | null => {
32-35: Avoid duplicate parseColor; reuse the global helperThere are two parseColor implementations (here and in global.helper.ts). Keep one source of truth.
Apply:
-import { parseIdFromUrl } from './global.helper'; +import { parseIdFromUrl, parseColor } from './global.helper'; @@ -export const getUserRatingColorRating = (el: HTMLElement): CSFDColorRating => { +export const getUserRatingColorRating = (el: HTMLElement): CSFDColorRating => { const color = parseColor(el.querySelector('td.name .icon').classNames.split(' ').pop() as Colors); return color; }; @@ -const parseColor = (quality: Colors): CSFDColorRating => { - switch (quality) { - case 'lightgrey': - return 'unknown'; - case 'red': - return 'good'; - case 'blue': - return 'average'; - case 'grey': - return 'bad'; - default: - return 'unknown'; - } -}; +// moved to global.helper.ts
1-16: Selector safetyMost querySelector calls assume elements exist; consider null‑checks to prevent runtime errors when the DOM shape varies.
src/vars.ts (1)
13-15: URL-encodedistrictandperiodto avoid malformed URLs.Safe even if they’re numbers/unions.
Apply this diff:
-export const cinemasUrl = (district: number | string, period: CSFDCinemaPeriod): string => { - return `https://www.csfd.cz/kino/?period=${period}&district=${district}`; -}; +export const cinemasUrl = (district: number | string, period: CSFDCinemaPeriod): string => { + return `https://www.csfd.cz/kino/?period=${encodeURIComponent(period)}&district=${encodeURIComponent(String(district))}`; +};tests/services.test.ts (2)
22-22: Avoid asyncdescribeblocks.
describeshould be synchronous; move async work into tests or hooks.Apply:
-describe('AllPages', async () => { +describe('AllPages', () => {
74-76: Remove noisyconsole.login tests.It pollutes CI output.
Apply:
- console.log(results);tsconfig.esm.json (1)
4-6: Use NodeNext module settings for true Node ESM resolution.Aligns TS resolver with Node’s ESM semantics in hybrid packages.
Apply this diff:
- "module": "ESNext", - "moduleResolution": "node", + "module": "NodeNext", + "moduleResolution": "NodeNext",Confirm your supported Node/TS versions match NodeNext semantics.
tests/fetchers.test.ts (1)
77-83: Make assertion resilient to extra fields.Use partial match to avoid brittleness if more props are added.
Apply:
- expect(creator.films.find((film) => film.title === 'Hej-rup!')).toEqual<CSFDCreatorScreening>({ - id: 3106, - title: 'Hej-rup!', - year: 1934, - colorRating: 'good' - }); + expect(creator.films.find((film) => film.title === 'Hej-rup!')).toEqual( + expect.objectContaining({ + id: 3106, + title: 'Hej-rup!', + year: 1934, + colorRating: 'good' + }) + );src/services/cinema.service.ts (1)
14-15: Initialize class field to satisfy strict property initialization.Prevents TS error when
strictPropertyInitializationis enabled.Apply:
- private cinema: CSFDCinema[]; + private cinema: CSFDCinema[] = [];tests/creator.test.ts (3)
148-150: PrefertoBeNull()over castingnulltoanyThis expectation should assert nullability directly; the current cast hides type issues and weakens the test.
Apply this diff:
- const creator = getCreatorBio(asideNodeComposer); - expect(creator).toEqual<string>(null as any); + const creator = getCreatorBio(asideNodeComposer); + expect(creator).toBeNull();
171-174: Fix test name and use null-safe matcherThe test is asserting age but titled “Birthplace”. Also prefer
toBeNull().Apply this diff:
- test('Birthplace', () => { - const creator = getCreatorBirthdayInfo(asideNodeComposer)?.age; - expect(creator).toEqual(null); - }); + test('Age', () => { + const creator = getCreatorBirthdayInfo(asideNodeComposer)?.age; + expect(creator).toBeNull(); + });
29-45: Consider tightening type expectations (optional)Many assertions use generics on
expect<T>, which provide no runtime benefit and can mask type drift. UsingtoBe/toEqualwithout generics keeps tests cleaner.Also applies to: 50-62, 67-79, 86-102, 107-119, 124-136, 143-155, 162-174, 179-182
tests/cinema.helper.test.ts (1)
76-86: Date parameter looks unused — either remove it or pass a real date
getCinemaFilms('', table)passes an empty date. If the helper doesn’t usedate, consider removing that parameter; otherwise pass a representative date to guard against future reliance on it.Would you like me to prepare a follow-up PR to either (a) drop the
dateparameter fromgetCinemaFilms, or (b) thread an actual date fromgetGroupedFilmsByDateinto this test?src/services/creator.service.ts (1)
26-36: Avoid repeated DOM parsing and use nullish coalescing for ageCache birthday info once;
|| nullcan incorrectly coerce0(use?? null). Also minor nit: reuseidyou already computed.Apply this diff:
- private buildCreator(id: number, asideEl: HTMLElement, filmsNode: HTMLElement) { - this.person = { + private buildCreator(id: number, asideEl: HTMLElement, filmsNode: HTMLElement) { + const info = getCreatorBirthdayInfo(asideEl); + this.person = { id, - name: getCreatorName(asideEl), - birthday: getCreatorBirthdayInfo(asideEl)?.birthday, - birthplace: getCreatorBirthdayInfo(asideEl)?.birthPlace, - photo: getCreatorPhoto(asideEl), - age: getCreatorBirthdayInfo(asideEl)?.age || null, - bio: getCreatorBio(asideEl), + name: getCreatorName(asideEl), + birthday: info?.birthday || '', + birthplace: info?.birthPlace || '', + photo: getCreatorPhoto(asideEl), + age: info?.age ?? null, + bio: getCreatorBio(asideEl), films: getCreatorFilms(filmsNode) }; }src/services/search.service.ts (2)
73-91: Rename variable fromusertoseriesin TV-series loopMinor readability/consistency fix; the variable holds a TV series item, not a user.
Apply this diff:
- tvSeriesNode.forEach((m) => { - const url = getSearchUrl(m); - - const user: CSFDSearchMovie = { + tvSeriesNode.forEach((m) => { + const url = getSearchUrl(m); + + const series: CSFDSearchMovie = { id: parseIdFromUrl(url), title: getSearchTitle(m), year: getSearchYear(m), url: `https://www.csfd.cz${url}`, type: getSearchType(m), colorRating: getSearchColorRating(m), poster: getSearchPoster(m), origins: getSearchOrigins(m), creators: { directors: parseSearchPeople(m, 'directors'), actors: parseSearchPeople(m, 'actors') } }; - tvSeries.push(user); + tvSeries.push(series); });
36-58: DRY up movie/series parsing (optional)Movie and TV-series parsing are identical. Extract a small
parseMovieNode(m: HTMLElement): CSFDSearchMovieto reduce duplication and future drift.I can draft the helper and inline replacements if you want it in this PR.
Also applies to: 73-91
tests/user-ratings.test.ts (3)
113-119: UsetoBefor primitives and drop genericsCleaner and idiomatic for string unions; generics on
expectadd noise.Apply this diff:
- const movie = getUserRatingColorRating(movies[3]); - expect(movie).toEqual<CSFDColorRating>('average'); + const movie = getUserRatingColorRating(movies[3]); + expect(movie).toBe('average');
135-141: Same here:toBeis sufficientStrings are primitives;
toBeis fine and reads better.Apply this diff:
- const movie = getUserRatingUrl(movies[0]); - expect(movie).toEqual<string>('https://www.csfd.cz/film/1254361-stutz/'); + const movie = getUserRatingUrl(movies[0]); + expect(movie).toBe('https://www.csfd.cz/film/1254361-stutz/');
104-111: Consider re-enabling the gray/black rating testsThey’re useful guardrails for color mapping regressions once stable.
I can help adjust fixtures/parsers to make these deterministic and re-enable them.
src/services/movie.service.ts (2)
29-29: Definite assignment forfilmfieldAvoids TS strict initialization warnings and clarifies lifecycle.
- private film: CSFDMovie; + private film!: CSFDMovie;
63-64: Avoid unsafe cast oftype; validate against allowed enum valuesCasting blindly may leak unexpected strings to public API if the markup changes.
- type: getMovieType(el) as CSFDFilmTypes, + type: toFilmType(getMovieType(el)),Add helper (outside the class):
const FILM_TYPES: readonly CSFDFilmTypes[] = [ 'film','TV film','pořad','seriál','divadelní záznam','koncert','série', 'studentský film','amatérský film','hudební videoklip','epizoda' ] as const; const toFilmType = (t: string): CSFDFilmTypes => (FILM_TYPES as readonly string[]).includes(t) ? (t as CSFDFilmTypes) : 'film';package.json (1)
9-12: Build pipeline simplification (optional)You run
tscthree times. If you use project references,tsc -bcan orchestrate both CJS/ESM builds and speed up incremental builds.Example:
- "build": "tsc && tsc -p tsconfig.cjs.json && tsc -p tsconfig.esm.json && yarn barrels", + "build": "tsc -b tsconfig.cjs.json tsconfig.esm.json && yarn barrels",Also applies to: 17-17
tests/search.test.ts (1)
98-117: Add one negative-path test for robustnessConsider a single test case per group where the queried node misses an expected selector (e.g., no
.film-title-name, no icon). This will guard against future regressions once null-safety is added in helpers.I can craft minimal HTML snippets with missing nodes and add assertions for empty string/NaN (or null if you choose that contract). Want me to open a follow-up PR with those tests?
Also applies to: 252-265
src/helpers/cinema.helper.ts (3)
11-13: Null-safe color ratingMake this tolerant to missing icons and keep behavior stable when the element is absent.
-export const getCinemaColorRating = (el: HTMLElement): CSFDColorRating => { - return parseColor(el?.classNames.split(' ').pop() as Colors); -}; +export const getCinemaColorRating = (el: HTMLElement | null): CSFDColorRating => { + if (!el) return 'unknown'; + return parseColor((el.classNames as any)?.split?.(' ')?.pop() as Colors); +};Node-html-parser’s classNames can be string or string[] depending on version. Please confirm the version used in this repo. If it’s string[], we can switch to
Array.isArray(el.classNames) ? el.classNames.slice(-1)[0] : ....
66-67: Remove unused parameter in getCinemaFilms; simplify call
dateisn’t used. Drop it to avoid confusion.- return { date: dateText, films: getCinemaFilms('', films) }; + return { date: dateText, films: getCinemaFilms(films) };
72-93: Null checks and safer ID parsing in getCinemaFilmsGuard against
elbeing null/undefined and missing anchors.-export const getCinemaFilms = (date: string, el: HTMLElement | null): CSFDCinemaMovie[] => { - const filmNodes = el.querySelectorAll('.cinema-table tr'); +export const getCinemaFilms = (el: HTMLElement | null): CSFDCinemaMovie[] => { + if (!el) return []; + const filmNodes = el?.querySelectorAll('.cinema-table tr') ?? []; const films = filmNodes.map((filmNode) => { - const url = filmNode.querySelector('td.name h3 a')?.attributes.href; - const id = getCinemaUrlId(url); + const url = filmNode.querySelector('td.name h3 a')?.attributes?.href; + const id = getCinemaUrlId(url || ''); const title = filmNode.querySelector('.name h3')?.text.trim(); - const colorRating = getCinemaColorRating(filmNode.querySelector('.name .icon')); + const colorRating = getCinemaColorRating(filmNode.querySelector('.name .icon')); const showTimes = filmNode.querySelectorAll('.td-time')?.map((x) => x.textContent.trim()); const meta = filmNode.querySelectorAll('.td-title span')?.map((x) => x.text.trim()); return { id, title, url, colorRating, showTimes, meta: parseMeta(meta) }; }); return films; };src/dto/movie.ts (1)
49-59: Type rename is breaking; add BC aliasDownstream users may still import CSFDCreator. Provide a harmless alias.
export interface CSFDCreators { directors: CSFDMovieCreator[]; writers: CSFDMovieCreator[]; cinematography: CSFDMovieCreator[]; music: CSFDMovieCreator[]; actors: CSFDMovieCreator[]; basedOn: CSFDMovieCreator[]; producers: CSFDMovieCreator[]; filmEditing: CSFDMovieCreator[]; costumeDesign: CSFDMovieCreator[]; productionDesign: CSFDMovieCreator[]; }Add right after CSFDMovieCreator is declared:
export interface CSFDMovieCreator { /** * CSFD person ID. * ... */ id: number; name: string; url: string; } + +// Backward compatibility alias (kept for external consumers) +export type CSFDCreator = CSFDMovieCreator;src/helpers/creator.helper.ts (4)
7-9: Null-safe rating extractionPrevent crashes when the icon is missing.
-const getCreatorColorRating = (el: HTMLElement): CSFDColorRating => { - return parseColor(el?.classNames.split(' ').pop() as Colors); -}; +const getCreatorColorRating = (el: HTMLElement | null): CSFDColorRating => { + if (!el) return 'unknown'; + return parseColor((el.classNames as any)?.split?.(' ')?.pop() as Colors); +};
18-20: Null guard for getCreatorNameAvoid NPE when
elis null.-export const getCreatorName = (el: HTMLElement | null): string => { - return el.querySelector('h1').innerText.trim(); -}; +export const getCreatorName = (el: HTMLElement | null): string => { + return el?.querySelector('h1')?.innerText?.trim() ?? ''; +};
45-47: Return type can be null
getCreatorBioreturnsnullwhen not found but is typed asstring.-export const getCreatorBio = (el: HTMLElement | null): string => { - return el.querySelector('.article-content p')?.text.trim().split('\n')[0].trim() || null; -}; +export const getCreatorBio = (el: HTMLElement | null): string | null => { + return el?.querySelector('.article-content p')?.text?.trim().split('\n')[0].trim() ?? null; +};
49-52: Null guard for photoPrevent crashes and reflect nullable return.
-export const getCreatorPhoto = (el: HTMLElement | null): string => { - const image = el.querySelector('img').attributes.src; - return addProtocol(image); -}; +export const getCreatorPhoto = (el: HTMLElement | null): string | null => { + const image = el?.querySelector('img')?.attributes?.src; + return image ? addProtocol(image) : null; +};tests/movie.test.ts (1)
592-594: Prefer toBeNull for clarity and type-safetyThis expresses the intent better and avoids odd generics.
- const movie = getMovieRatingCount(movieNode); - expect(movie).toEqual<CSFDMovieListItem[]>(null as any); + const movie = getMovieRatingCount(movieNode); + expect(movie).toBeNull();src/helpers/movie.helper.ts (7)
72-96: Guard fallback path in getMovieDuration; fix typoAvoid NPE when
.originis missing and correct comment.} catch (error) { - const origin = el.querySelector('.origin').innerText; + const originNode = el.querySelector('.origin') as HTMLElement | null; + const origin = originNode?.innerText; + if (!origin) return null; const timeString = origin.split(','); if (timeString.length > 2) { - // Get last time elelment + // Get last time element const timeString2 = timeString.pop().trim(); // Clean it const timeRaw = timeString2.split('(')[0].trim();
98-121: Optional chaining on auxiliary fields in getMovieTitlesOtherPrevents crashes when flags or text are missing.
const titlesOther = namesNode.map((el) => { - const country = el.querySelector('img.flag').attributes.alt; - const title = el.textContent.trim().split('\n')[0]; + const country = el.querySelector('img.flag')?.attributes?.alt; + const title = el.textContent?.trim().split('\n')[0]; if (country && title) { return { country, title }; } else { return null; } });
122-137: Null guard for getMoviePosterAccepts
HTMLElement | nullbut doesn’t guard.export const getMoviePoster = (el: HTMLElement | null): string => { - const poster = el.querySelector('.film-posters img'); + if (!el) return null; + const poster = el.querySelector('.film-posters img');
139-147: Null guard for getMovieRandomPhotoexport const getMovieRandomPhoto = (el: HTMLElement | null): string => { - const imageNode = el.querySelector('.gallery-item picture img'); + if (!el) return null; + const imageNode = el.querySelector('.gallery-item picture img');
149-156: Null guard for getMovieTriviaexport const getMovieTrivia = (el: HTMLElement | null): string[] => { - const triviaNodes = el.querySelectorAll('.article-trivia ul li'); + if (!el) return null; + const triviaNodes = el.querySelectorAll('.article-trivia ul li');
210-215: getBoxContent return type should include undefinedIt can return
undefinedwhen the box isn’t found.-const getBoxContent = (el: HTMLElement, box: string): HTMLElement => { +const getBoxContent = (el: HTMLElement, box: string): HTMLElement | undefined => {
233-251: Guard premiere parsing against missing nodesAvoid NPEs on pages without titles or flags.
export const getMoviePremieres = (el: HTMLElement): CSFDPremiere[] => { const premiereNodes = el.querySelectorAll('.box-premieres li'); const premiere: CSFDPremiere[] = []; for (const premiereNode of premiereNodes) { - const title = premiereNode.querySelector('p + span').attributes.title; - - if (title) { - const [date, ...company] = title?.split(' '); - - premiere.push({ - country: premiereNode.querySelector('.flag')?.attributes.title || null, - format: premiereNode.querySelector('p').textContent.trim()?.split(' od')[0], - date, - company: company.join(' ') - }); - } + const span = premiereNode.querySelector('p + span'); + const title = span?.attributes?.title; + if (!title) continue; + const [date, ...company] = title.split(' '); + premiere.push({ + country: premiereNode.querySelector('.flag')?.attributes?.title || null, + format: premiereNode.querySelector('p')?.textContent?.trim()?.split(' od')[0], + date, + company: company.join(' ') + }); } return premiere; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
barrels.json(1 hunks)package.json(4 hunks)server.ts(1 hunks)src/dto/cinema.ts(1 hunks)src/dto/movie.ts(1 hunks)src/dto/search.ts(2 hunks)src/helpers/cinema.helper.ts(3 hunks)src/helpers/creator.helper.ts(2 hunks)src/helpers/global.helper.ts(1 hunks)src/helpers/movie.helper.ts(13 hunks)src/helpers/search.helper.ts(1 hunks)src/helpers/user-ratings.helper.ts(2 hunks)src/index.ts(1 hunks)src/services/cinema.service.ts(2 hunks)src/services/creator.service.ts(2 hunks)src/services/movie.service.ts(2 hunks)src/services/search.service.ts(3 hunks)src/services/user-ratings.service.ts(3 hunks)src/types.ts(1 hunks)src/vars.ts(1 hunks)tests/cinema.helper.test.ts(5 hunks)tests/cinema.service.test.ts(1 hunks)tests/creator.test.ts(7 hunks)tests/fetchers.test.ts(1 hunks)tests/movie.test.ts(13 hunks)tests/search.test.ts(8 hunks)tests/services.test.ts(1 hunks)tests/user-ratings.test.ts(2 hunks)tsconfig.cjs.json(1 hunks)tsconfig.esm.json(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
src/services/user-ratings.service.ts (2)
src/helpers/user-ratings.helper.ts (7)
getUserRatingType(18-22)getUserRatingId(6-9)getUserRatingTitle(24-26)getUserRatingYear(28-30)getUserRatingUrl(41-44)getUserRatingColorRating(32-35)getUserRatingDate(37-39)src/dto/global.ts (1)
CSFDColorRating(21-21)
src/services/movie.service.ts (3)
src/helpers/movie.helper.ts (19)
getMovieTitle(21-23)getMovieYear(62-70)getMovieDuration(72-96)getMovieDescriptions(158-162)getMovieGenres(25-28)getMovieType(190-193)getMovieOrigins(30-34)getMovieColorRating(36-38)getMovieRating(40-50)getMovieRatingCount(52-60)getMovieTitlesOther(98-120)getMoviePoster(122-137)getMovieRandomPhoto(139-147)getMovieTrivia(149-156)getMovieGroup(180-188)getMovieVods(195-208)getMovieTags(253-256)getMoviePremieres(233-251)getMovieBoxMovies(217-231)src/dto/global.ts (1)
CSFDFilmTypes(25-36)src/vars.ts (1)
movieUrl(7-8)
src/helpers/search.helper.ts (4)
src/dto/global.ts (2)
CSFDFilmTypes(25-36)CSFDColorRating(21-21)src/helpers/global.helper.ts (2)
parseColor(29-42)addProtocol(44-46)src/dto/user-ratings.ts (1)
Colors(21-21)src/dto/movie.ts (1)
CSFDMovieCreator(66-77)
src/services/search.service.ts (3)
src/helpers/search.helper.ts (8)
getSearchUrl(22-24)getSearchTitle(14-16)getSearchYear(18-20)getSearchType(9-12)getSearchColorRating(26-30)getSearchPoster(32-35)getSearchOrigins(37-42)parseSearchPeople(44-66)src/dto/search.ts (1)
CSFDSearchMovie(11-15)src/helpers/global.helper.ts (1)
parseIdFromUrl(4-12)
src/services/creator.service.ts (1)
src/helpers/creator.helper.ts (5)
getCreatorName(18-20)getCreatorBirthdayInfo(22-43)getCreatorPhoto(49-52)getCreatorBio(45-47)getCreatorFilms(66-95)
src/dto/search.ts (1)
src/dto/movie.ts (1)
CSFDMovieCreator(66-77)
tests/search.test.ts (3)
src/helpers/search.helper.ts (8)
getSearchTitle(14-16)getSearchYear(18-20)getSearchUrl(22-24)getSearchType(9-12)getSearchColorRating(26-30)getSearchPoster(32-35)getSearchOrigins(37-42)parseSearchPeople(44-66)src/dto/global.ts (2)
CSFDFilmTypes(25-36)CSFDColorRating(21-21)src/dto/movie.ts (1)
CSFDMovieCreator(66-77)
src/helpers/user-ratings.helper.ts (3)
src/dto/global.ts (2)
CSFDFilmTypes(25-36)CSFDColorRating(21-21)src/helpers/global.helper.ts (1)
parseColor(29-42)src/dto/user-ratings.ts (1)
Colors(21-21)
src/services/cinema.service.ts (1)
src/helpers/cinema.helper.ts (1)
getCinemaCoords(27-45)
src/helpers/creator.helper.ts (4)
src/dto/global.ts (1)
CSFDColorRating(21-21)src/helpers/global.helper.ts (3)
parseColor(29-42)parseIdFromUrl(4-12)addProtocol(44-46)src/dto/user-ratings.ts (1)
Colors(21-21)src/dto/creator.ts (1)
CSFDCreatorScreening(14-14)
tests/cinema.helper.test.ts (1)
src/helpers/cinema.helper.ts (3)
getCinemaUrlId(20-25)getCinemaCoords(27-45)getCinemaFilms(72-93)
tests/user-ratings.test.ts (2)
src/helpers/user-ratings.helper.ts (7)
getUserRatingId(6-9)getUserRatingType(18-22)getUserRatingTitle(24-26)getUserRatingYear(28-30)getUserRatingColorRating(32-35)getUserRatingDate(37-39)getUserRatingUrl(41-44)src/dto/global.ts (2)
CSFDFilmTypes(25-36)CSFDColorRating(21-21)
tests/movie.test.ts (5)
src/index.ts (1)
movie(28-30)src/services/movie.service.ts (1)
movie(31-47)src/helpers/movie.helper.ts (19)
getMovieId(16-19)getMovieTitle(21-23)getMoviePoster(122-137)getMovieRandomPhoto(139-147)getMovieTrivia(149-156)getMovieDuration(72-96)getMovieVods(195-208)getMovieTitlesOther(98-120)getMovieOrigins(30-34)getMovieDescriptions(158-162)getMovieGenres(25-28)getMovieType(190-193)getMovieYear(62-70)getMovieRatingCount(52-60)getMovieRating(40-50)getMovieColorRating(36-38)getMovieGroup(180-188)getMoviePremieres(233-251)getMovieBoxMovies(217-231)src/dto/movie.ts (5)
CSFDVod(43-46)CSFDTitlesOther(61-64)CSFDMovieCreator(66-77)CSFDPremiere(141-146)CSFDMovieListItem(79-83)src/dto/global.ts (1)
CSFDColorRating(21-21)
tests/creator.test.ts (2)
src/helpers/creator.helper.ts (5)
getCreatorName(18-20)getCreatorBio(45-47)getCreatorPhoto(49-52)getCreatorBirthdayInfo(22-43)getCreatorFilms(66-95)src/dto/creator.ts (1)
CSFDCreatorScreening(14-14)
src/helpers/movie.helper.ts (3)
src/helpers/global.helper.ts (2)
parseIdFromUrl(4-12)getColor(14-27)src/dto/movie.ts (8)
CSFDGenres(85-127)CSFDTitlesOther(61-64)CSFDMovieCreator(66-77)CSFDCreatorGroups(129-139)CSFDVod(43-46)CSFDBoxContent(148-148)CSFDMovieListItem(79-83)CSFDPremiere(141-146)src/dto/global.ts (1)
CSFDColorRating(21-21)
src/helpers/cinema.helper.ts (3)
src/dto/global.ts (1)
CSFDColorRating(21-21)src/helpers/global.helper.ts (1)
parseIdFromUrl(4-12)src/dto/cinema.ts (1)
CSFDCinemaMovie(18-21)
🔇 Additional comments (15)
src/helpers/global.helper.ts (1)
1-2: Import path migration to DTOs looks goodNo runtime impact; aligns with the DTO move.
server.ts (1)
6-6: DTO import path update is OKMatches the refactor; no behavior change.
src/dto/cinema.ts (1)
1-1: Import path switch to DTO movie is correctConsistent with the DTO migration; no behavioral change.
tsconfig.cjs.json (1)
1-7: Disable declaration emission for the CJS build0 .d.ts found in dist/types — inconclusive; still recommend disabling declarations here (or run ESM and CJS builds sequentially) to avoid emit races/overwrites.
Apply to tsconfig.cjs.json:
{ "extends": "./tsconfig.json", "compilerOptions": { "module": "CommonJS", - "outDir": "./dist/cjs" + "outDir": "./dist/cjs", + "declaration": false } }src/vars.ts (1)
1-1: LGTM on DTO import path update.tests/services.test.ts (1)
2-2: LGTM on DTO import migration.tests/fetchers.test.ts (1)
3-4: LGTM on DTO import migration.src/types.ts (1)
1-6: Centralized DTO re‑exports look good.Keeps the public type surface tidy.
src/services/cinema.service.ts (1)
2-2: LGTM on DTO/helper migrations.Imports and
coordssource updated consistently.Also applies to: 6-7, 41-41
src/dto/search.ts (1)
2-2: LGTM on moving toCSFDMovieCreator.Type surface aligns with movie DTOs.
Also applies to: 25-27, 30-32
tests/cinema.helper.test.ts (1)
45-56: Good coverage of happy- and null-paths for coordsPositive case + no-
linkMapsElcase are both covered. Nice.tests/user-ratings.test.ts (1)
35-43: IDs tests look solidCovers both first and last entries; no concerns.
src/index.ts (1)
36-38: LGTM: newsearchfacade wiring looks cleanConfirm no external docs/readme examples construct
new Csfd(...). If they do, update them or add a deprecation note.tests/search.test.ts (1)
3-15: LGTM: tests reflect the new Search helpers and DTO typesGood coverage across titles/years/urls/types/colors/posters/origins/people for movies and TV series. This gives confidence for the rename/refactor.
Also applies to: 23-334
src/helpers/cinema.helper.ts (1)
20-25: LGTM: graceful handling of empty URLReturning
nullon falsy input aligns with downstream expectations.
| "compilerOptions": { | ||
| "target": "es2017", | ||
| "target": "es2015", | ||
| "lib": ["dom"], |
There was a problem hiding this comment.
lib excludes ES libs; Promise/Array types may break
Setting "lib": ["dom"] overrides defaults and drops ES libs. Add an ES lib to match target.
Apply:
- "lib": ["dom"],
+ "lib": ["ES2019", "DOM"],📝 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.
| "lib": ["dom"], | |
| "lib": ["ES2019", "DOM"], |
🤖 Prompt for AI Agents
In tsconfig.json around line 4, the "lib" value only lists "dom" which overrides
defaults and omits ECMAScript library declarations (so Promise/Array etc. can be
missing); update the "lib" array to include an ES lib that matches your target
(for example add "es2019" or "es6" alongside "dom", e.g. ["es2019","dom"]),
ensuring the chosen ES lib version aligns with the "target" setting.
|
Here's the code health analysis summary for commits Analysis Summary
|
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/services/user-ratings.service.ts (2)
33-35: Potential crash when pagination is absent.Optional chaining is ineffective because pagesNode.childNodes is accessed unguarded in the index expression. This can throw on single-page profiles.
Apply a safe parse:
- const pagesNode = items.querySelector('.pagination'); - const pages = +pagesNode?.childNodes[pagesNode.childNodes.length - 4].rawText || 1; + const pagesNode = items.querySelector('.pagination'); + let pages = 1; + if (pagesNode) { + const children = pagesNode.childNodes ?? []; + // Heuristic: last numeric item before "next" control + const idx = Math.max(0, children.length - 4); + const text = (children[idx] as any)?.rawText?.trim?.() ?? ''; + const n = parseInt(text, 10); + pages = Number.isFinite(n) ? n : 1; + }
21-29: Reset accumulator per call to avoid cross-call leakage.this.films is never cleared; repeated invocations will accumulate stale results.
public async userRatings( user: string | number, config?: CSFDUserRatingConfig ): Promise<CSFDUserRatings[]> { - let allMovies: CSFDUserRatings[] = []; + this.films = []; + let allMovies: CSFDUserRatings[] = [];tests/fetchers.test.ts (2)
17-19: Remove generics on matchers; Vitest/Jest matchers aren’t generic.Type args like , , <string[]>, on toEqual/toContain aren’t part of the matcher APIs and can cause TS errors.
Apply this diff snippet pattern (representative lines shown):
- expect(movieSelected?.year).toEqual<number>(2025); + expect(movieSelected?.year).toEqual(2025); - expect(movieSelected?.userDate).toContain<string>('2025'); + expect(movieSelected?.userDate).toContain('2025'); - expect(movie.type).toEqual<CSFDFilmTypes>('film'); + expect(movie.type).toEqual('film'); - expect(movie.colorRating).toEqual<CSFDColorRating>('good'); + expect(movie.colorRating).toEqual('good'); - expect(movie.origins[0]).toEqual<string>('USA'); + expect(movie.origins[0]).toEqual('USA'); - expect(movie.year).toEqual<number>(1994); + expect(movie.year).toEqual(1994); - expect(matrix?.creators?.directors.map((x) => x.name)).toEqual<string[]>( + expect(matrix?.creators?.directors.map((x) => x.name)).toEqual( expect.arrayContaining(['Lilly Wachowski']) ); - expect(creator.films.find((film) => film.title === 'Hej-rup!')).toEqual<CSFDCreatorScreening>({ + expect(creator.films.find((film) => film.title === 'Hej-rup!')).toEqual({ id: 3106, title: 'Hej-rup!', year: 1934, colorRating: 'good' });Also applies to: 43-43, 46-46, 48-48, 52-53, 63-63, 77-82
94-94: Fix error assertions inside catch; use instance checks, not toThrow/toContain.
toThrowis for functions, not error instances;toContaindoesn’t apply to Error.Apply this diff:
- expect(e).toContain(Error); + expect(e).toBeInstanceOf(Error);- expect(e).toThrow(Error); + expect(e).toBeInstanceOf(Error);Also applies to: 106-106
src/helpers/creator.helper.ts (2)
22-43: getCreatorBirthdayInfo: null-safety and typings.Accessing
el.querySelectorwhenelis null will throw;agecan be null but typed as number.-export const getCreatorBirthdayInfo = ( - el: HTMLElement | null -): { birthday: string; age: number; birthPlace: string } => { - const infoBlock = el.querySelector('h1 + p'); +export const getCreatorBirthdayInfo = ( + el: HTMLElement | null +): { birthday: string | null; age: number | null; birthPlace: string | null } => { + const infoBlock = el?.querySelector('h1 + p'); const text = infoBlock?.innerHTML.trim(); @@ - let birthday: string = ''; + let birthday: string | null = null; @@ - const age = ageRow ? +parseAge(ageRow) : null; - const birthPlace = birthPlaceRow ? parseBirthPlace(birthPlaceRow) : ''; + const age = ageRow ? parseAge(ageRow) : null; + const birthPlace = birthPlaceRow ? parseBirthPlace(birthPlaceRow) : null; return { birthday, age, birthPlace }; };
66-95: getCreatorFilms: potential NPEs, uninitialized yearCache, and mixed return types.
el.querySelectorAll...whenelis null will throw.yearCacheused before set.- May return
{}which violates the return type.getCreatorColorRatingcan receive null icon.-export const getCreatorFilms = (el: HTMLElement | null): CSFDCreatorScreening[] => { - const filmNodes = el.querySelectorAll('.box')[0]?.querySelectorAll('table tr'); - let yearCache: number; - const films = filmNodes.map((filmNode) => { - const id = getCreatorId(filmNode.querySelector('td.name .film-title-name')?.attributes.href); - const title = filmNode.querySelector('.name')?.text.trim(); - const year = +filmNode.querySelector('.year')?.text.trim(); - const colorRating = getCreatorColorRating(filmNode.querySelector('.name .icon')); - - // Cache year from previous film because there is a gap between movies with same year - if (year) { - yearCache = +year; - } - - if (id && title) { - return { - id, - title, - year: year || yearCache, - colorRating - }; - } - return {}; - }); - // Remove empty objects - const filmsUnique = films.filter( - (value) => Object.keys(value).length !== 0 - ) as CSFDCreatorScreening[]; - return filmsUnique; -}; +export const getCreatorFilms = (el: HTMLElement | null): CSFDCreatorScreening[] => { + const rows = el?.querySelectorAll('.box')?.[0]?.querySelectorAll('table tr') ?? []; + let yearCache: number | null = null; + const out: CSFDCreatorScreening[] = []; + for (const row of rows) { + const id = getCreatorId(row.querySelector('td.name .film-title-name')?.getAttribute('href')); + const title = row.querySelector('.name')?.text?.trim(); + const yearText = row.querySelector('.year')?.text?.trim(); + const parsedYear = yearText ? Number(yearText) : NaN; + if (!Number.isNaN(parsedYear)) yearCache = parsedYear; + const year = Number.isNaN(parsedYear) ? yearCache : parsedYear; + const colorRating = getCreatorColorRating(row.querySelector('.name .icon')); + if (id != null && title && year != null) { + out.push({ id, title, year, colorRating }); + } + } + return out; +};src/helpers/cinema.helper.ts (1)
27-45: Harden map coords parsing; guard missing href; handle encoded/extra paramsCurrent split-based parsing breaks on encoded commas (%2C) and when extra params follow
q. Use URL/URLSearchParams and decode.export const getCinemaCoords = (el: HTMLElement | null): { lat: number; lng: number } | null => { if (!el) return null; const linkMapsEl = el.querySelector('a[href*="q="]'); if (!linkMapsEl) return null; - const linkMaps = linkMapsEl.getAttribute('href'); - const [_, latLng] = linkMaps.split('q='); - - const coords = latLng.split(','); - if (coords.length !== 2) return null; - - const lat = Number(coords[0]); - const lng = Number(coords[1]); - if (Number.isFinite(lat) && Number.isFinite(lng)) { - return { lat, lng }; - } - return null; + const href = linkMapsEl.getAttribute('href'); + if (!href) return null; + try { + const u = new URL(href, 'https://csfd.cz'); + const q = u.searchParams.get('q'); + if (!q) return null; + const [latStr, lngStr] = decodeURIComponent(q).split(','); + const lat = Number.parseFloat(latStr); + const lng = Number.parseFloat(lngStr); + if (Number.isFinite(lat) && Number.isFinite(lng)) { + return { lat, lng }; + } + } catch { + // ignore parse errors + } + return null; };
♻️ Duplicate comments (6)
src/helpers/creator.helper.ts (1)
11-16: Return-type mismatch in getCreatorId (returns null but typed number).Keep signature nullable and accept undefined inputs.
Apply:
-export const getCreatorId = (url: string): number => { - if (url) { - return parseIdFromUrl(url); - } - return null; -}; +export const getCreatorId = (url: string | null | undefined): number | null => { + return url ? parseIdFromUrl(url) : null; +};Additionally, align
parseIdFromUrl’s return type tonumber | nullin src/helpers/global.helper.ts:export const parseIdFromUrl = (url: string | null | undefined): number | null => { if (!url) return null; const idSlug = url.split('/')[2]; const id = idSlug?.split('-')[0]; return id ? +id : null; };Run to find and update any callers relying on non-null ids:
#!/bin/bash rg -nP "parseIdFromUrl\s*\(" -C2 rg -nP "getCreatorId\s*\(" -C2src/helpers/search.helper.ts (5)
14-16: Null‑safety for missing title node.Guard selector and trim result.
-export const getSearchTitle = (el: HTMLElement): string => { - return el.querySelector('.film-title-name').text; -}; +export const getSearchTitle = (el: HTMLElement): string => { + const n = el.querySelector('.film-title-name'); + return (n?.text ?? '').trim(); +};
18-20: Optional chaining misuse: can throw when year node is absent.
.replaceis accessed on possiblyundefined. Also make conversion resilient.-export const getSearchYear = (el: HTMLElement): number => { - return +el.querySelectorAll('.film-title-info .info')[0]?.innerText.replace(/[{()}]/g, ''); -}; +export const getSearchYear = (el: HTMLElement): number => { + const raw = el.querySelectorAll('.film-title-info .info')[0] + ?.innerText + ?.replace(/[{()}]/g, '') + ?.trim(); + const num = raw ? Number(raw) : NaN; + return Number.isFinite(num) ? num : NaN; +};
22-24: Null‑safety for URL.Avoid
.attributeson null.-export const getSearchUrl = (el: HTMLElement): string => { - return el.querySelector('.film-title-name').attributes.href; -}; +export const getSearchUrl = (el: HTMLElement): string => { + const href = el.querySelector('.film-title-name')?.attributes?.href; + return href ?? ''; +};
26-30: Harden color parsing when icon/class is missing.Prevent
.spliton undefined and provide a safe fallback.-export const getSearchColorRating = (el: HTMLElement): CSFDColorRating => { - return parseColor( - el.querySelector('.article-header i.icon').classNames.split(' ').pop() as Colors - ); -}; +export const getSearchColorRating = (el: HTMLElement): CSFDColorRating => { + const icon = el.querySelector('.article-header i.icon'); + const color = icon?.classNames?.split(' ')?.pop() as Colors | undefined; + return parseColor((color ?? 'lightgrey') as Colors); +};
32-35: Guard poster access.Avoid
.attributes.srcon null and keep return type stable.-export const getSearchPoster = (el: HTMLElement): string => { - const image = el.querySelector('img').attributes.src; - return addProtocol(image); -}; +export const getSearchPoster = (el: HTMLElement): string => { + const image = el.querySelector('img')?.attributes?.src; + return image ? addProtocol(image) : ''; +};
🧹 Nitpick comments (11)
tests/movie.test.ts (3)
592-594: Fix wrong generic/type in anomaly test.getMovieRatingCount returns number|null; the test asserts null but types it as CSFDMovieListItem[]. Use null assertion directly.
Apply:
- expect(movie).toEqual<CSFDMovieListItem[]>(null as any); + expect(movie).toBeNull();
380-387: Disambiguate duplicate test names.Two tests are named "Year". Give unique names to ease triage.
- test('Year', () => { + test('Year (movie)', () => { @@ - test('Year', () => { + test('Year (rich)', () => {
116-120: Either enable or remove commented-out tests.Keep the suite tidy or track with a TODO tag referencing an issue.
Also applies to: 136-139, 232-235
src/services/user-ratings.service.ts (3)
61-71: Make config optional in getPage signature.Call sites pass possibly undefined; the method already guards internally.
- private getPage(config: CSFDUserRatingConfig, movies: HTMLElement[]) { + private getPage(config: CSFDUserRatingConfig | undefined, movies: HTMLElement[]) {
39-55: Gate noisy logs behind a flag.Unconditional console logs when allPages is set can spam consumers.
- console.log('User', user, url); - console.log('Fetching all pages', pages); + if (config?.debug) { + console.log('User', user, url); + console.log('Fetching all pages', pages); + } @@ - console.log('Fetching page', i, 'out of', pages, '...'); + if (config?.debug) console.log('Fetching page', i, 'out of', pages, '...');
96-103: Minor: avoid redundant casts if helpers are typed.If getUserRatingColorRating/getUserRating already return CSFDColorRating/CSFDStars, drop the casts.
tests/fetchers.test.ts (1)
9-22: Quarantine “live” tests to reduce flakiness.These depend on a real site and can break CI; gate behind env or use
describe.runIf.Example:
-describe('Live: Fetch rating page', () => { +describe.runIf(!!process.env.CSFD_LIVE_TESTS)('Live: Fetch rating page', () => {Also applies to: 32-55, 58-67, 70-84
src/helpers/search.helper.ts (2)
37-42: Tidy optional chaining to avoid future pitfalls.You already guard
originsRaw; remove redundant?.and ensure.mapisn’t accessed on possibly undefined.- const originsRaw = el.querySelector('.article-content p .info')?.text; - if (!originsRaw) return []; - const originsAll = originsRaw?.split(', ')?.[0]; - return originsAll?.split('/').map((country) => country.trim()); + const originsRaw = el.querySelector('.article-content p .info')?.text; + if (!originsRaw) return []; + const originsAll = originsRaw.split(', ')[0]; + return originsAll.split('/').map((country) => country.trim());
44-66: Initialize discriminator deterministically and harden mapping.Prevents “used before assign” warnings and handles missing
href/text gracefully.-export const parseSearchPeople = (el: HTMLElement, type: 'directors' | 'actors'): CSFDMovieCreator[] => { - let who: Creator; - if (type === 'directors') who = 'Režie:'; - if (type === 'actors') who = 'Hrají:'; +export const parseSearchPeople = (el: HTMLElement, type: 'directors' | 'actors'): CSFDMovieCreator[] => { + const who: Creator = type === 'directors' ? 'Režie:' : 'Hrají:'; - const peopleNode = Array.from(el && el.querySelectorAll('.article-content p')).find((el) => - el.textContent.includes(who) - ); + const peopleNode = Array.from(el.querySelectorAll('.article-content p')).find((p) => + p.textContent?.includes(who) + ); if (peopleNode) { const people = Array.from(peopleNode.querySelectorAll('a')) as unknown as HTMLElement[]; return people.map((person) => { + const href = person.attributes?.href ?? ''; return { - id: parseIdFromUrl(person.attributes.href), - name: person.innerText.trim(), - url: `https://www.csfd.cz${person.attributes.href}` + id: parseIdFromUrl(href), + name: person.innerText?.trim() ?? '', + url: href ? `https://www.csfd.cz${href}` : '' }; }); } else { return []; } };tests/search.test.ts (2)
23-35: Reduce noise: avoid generics ontoEqualand consider table‑driven tests.Using
toEqual<T>doesn’t affect runtime and adds clutter. Prefer plain assertions orascasts. Considertest.eachto condense repetitive cases.Example for titles:
-const movie = getSearchTitle(moviesNode[0]); -expect(movie).toEqual<string>('Matrix'); +expect(getSearchTitle(moviesNode[0])).toEqual('Matrix');Example table:
test.each([ [0, 'Matrix'], [moviesNode.length - 1, 'Matrix hunter'], [5, 'Matrix - Reloaded'], ])('title at %d', (idx, expected) => { expect(getSearchTitle(moviesNode[idx])).toBe(expected); });Also applies to: 38-50, 53-66, 68-81, 83-96, 98-117, 119-136, 138-173, 177-190, 192-205, 207-220, 222-235, 237-250, 252-265, 267-284, 286-333
53-66: Add edge‑case tests for null‑safety (if you adopt the helper hardening).Cover missing nodes (e.g., no
.film-title-name, noimg, no year node) to lock desired behavior ('',NaN,[]).Proposed additions:
test('missing title returns empty string', () => { const article = parse('<article class="main-movies"></article>'); expect(getSearchTitle(article as any)).toBe(''); }); test('missing year returns NaN', () => { const article = parse('<article class="main-movies"><div class="film-title-info"><span class="info"></span></div></article>'); expect(Number.isNaN(getSearchYear(article as any))).toBe(true); }); test('missing poster returns empty string', () => { const article = parse('<article class="main-movies"></article>'); expect(getSearchPoster(article as any)).toBe(''); });Also applies to: 98-117, 252-265
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
barrels.json(1 hunks)package.json(4 hunks)server.ts(1 hunks)src/dto/cinema.ts(1 hunks)src/dto/movie.ts(1 hunks)src/dto/search.ts(2 hunks)src/helpers/cinema.helper.ts(3 hunks)src/helpers/creator.helper.ts(2 hunks)src/helpers/global.helper.ts(1 hunks)src/helpers/movie.helper.ts(13 hunks)src/helpers/search.helper.ts(1 hunks)src/helpers/user-ratings.helper.ts(2 hunks)src/index.ts(1 hunks)src/services/cinema.service.ts(2 hunks)src/services/creator.service.ts(2 hunks)src/services/movie.service.ts(2 hunks)src/services/search.service.ts(3 hunks)src/services/user-ratings.service.ts(3 hunks)src/types.ts(1 hunks)src/vars.ts(1 hunks)tests/cinema.helper.test.ts(5 hunks)tests/cinema.service.test.ts(1 hunks)tests/creator.test.ts(7 hunks)tests/fetchers.test.ts(1 hunks)tests/movie.test.ts(13 hunks)tests/search.test.ts(8 hunks)tests/services.test.ts(1 hunks)tests/user-ratings.test.ts(2 hunks)tsconfig.cjs.json(1 hunks)tsconfig.esm.json(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/services.test.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- src/types.ts
- server.ts
- src/helpers/global.helper.ts
- src/dto/movie.ts
- src/vars.ts
- tsconfig.cjs.json
- tests/cinema.service.test.ts
- src/services/creator.service.ts
- src/dto/search.ts
- tsconfig.json
- tsconfig.esm.json
- src/dto/cinema.ts
- barrels.json
- src/services/cinema.service.ts
- src/services/movie.service.ts
- tests/creator.test.ts
- package.json
- tests/user-ratings.test.ts
- src/helpers/movie.helper.ts
- src/helpers/user-ratings.helper.ts
- tests/cinema.helper.test.ts
- src/services/search.service.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/services/user-ratings.service.ts (2)
src/helpers/user-ratings.helper.ts (7)
getUserRatingType(18-22)getUserRatingId(6-9)getUserRatingTitle(24-26)getUserRatingYear(28-30)getUserRatingUrl(41-44)getUserRatingColorRating(32-35)getUserRatingDate(37-39)src/dto/global.ts (1)
CSFDColorRating(21-21)
tests/search.test.ts (3)
src/helpers/search.helper.ts (8)
getSearchTitle(14-16)getSearchYear(18-20)getSearchUrl(22-24)getSearchType(9-12)getSearchColorRating(26-30)getSearchPoster(32-35)getSearchOrigins(37-42)parseSearchPeople(44-66)src/dto/global.ts (2)
CSFDFilmTypes(25-36)CSFDColorRating(21-21)src/dto/movie.ts (1)
CSFDMovieCreator(66-77)
src/helpers/search.helper.ts (4)
src/dto/global.ts (2)
CSFDFilmTypes(25-36)CSFDColorRating(21-21)src/helpers/global.helper.ts (2)
parseColor(29-42)addProtocol(44-46)src/dto/user-ratings.ts (1)
Colors(21-21)src/dto/movie.ts (1)
CSFDMovieCreator(66-77)
tests/movie.test.ts (4)
src/services/movie.service.ts (1)
movie(31-47)src/helpers/movie.helper.ts (19)
getMovieId(16-19)getMovieTitle(21-23)getMoviePoster(122-137)getMovieRandomPhoto(139-147)getMovieTrivia(149-156)getMovieDuration(72-96)getMovieVods(195-208)getMovieTitlesOther(98-120)getMovieOrigins(30-34)getMovieDescriptions(158-162)getMovieGenres(25-28)getMovieType(190-193)getMovieYear(62-70)getMovieRatingCount(52-60)getMovieRating(40-50)getMovieColorRating(36-38)getMovieGroup(180-188)getMoviePremieres(233-251)getMovieBoxMovies(217-231)src/dto/movie.ts (5)
CSFDVod(43-46)CSFDTitlesOther(61-64)CSFDMovieCreator(66-77)CSFDPremiere(141-146)CSFDMovieListItem(79-83)src/dto/global.ts (1)
CSFDColorRating(21-21)
src/helpers/creator.helper.ts (4)
src/dto/global.ts (1)
CSFDColorRating(21-21)src/helpers/global.helper.ts (3)
parseColor(29-42)parseIdFromUrl(4-12)addProtocol(44-46)src/dto/user-ratings.ts (1)
Colors(21-21)src/dto/creator.ts (1)
CSFDCreatorScreening(14-14)
src/helpers/cinema.helper.ts (3)
src/dto/global.ts (1)
CSFDColorRating(21-21)src/helpers/global.helper.ts (1)
parseIdFromUrl(4-12)src/dto/cinema.ts (1)
CSFDCinemaMovie(18-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (11)
tests/movie.test.ts (2)
3-12: DTO-based imports look good.Types are correctly sourced from dto/* and helper import paths align with the refactor.
13-32: Helper migration is consistent and behavior-preserving.getX → getMovieX renames are applied uniformly; expectations match the helper contracts shown in dto/helper snippets.
Also applies to: 113-161, 166-185, 189-212, 216-235, 239-281, 303-316, 320-327, 331-347, 351-362, 366-377, 381-396, 400-416, 420-430, 447-485, 489-550, 555-571, 577-587
src/services/user-ratings.service.ts (1)
2-4: DTO and helper import updates are correct.Aligned with the DTO surface; no issues.
Also applies to: 7-15
src/index.ts (3)
1-5: DTO export migration is correct.Public types now sourced from dto/*; consistent with the refactor.
13-19: Constructor wiring and new search() API look good.Dependency order matches instantiation; method forwards correctly.
Also applies to: 36-38
51-57: Factory instantiation order is correct.Matches constructor signature; safe for both ESM/CJS entrypoints.
tests/fetchers.test.ts (1)
3-4: DTO import path updates look good.Consistent with the repo-wide move to dto/*.
Please confirm barrel exports (src/index) still re-export these DTO types for consumers if that’s intended.
src/helpers/creator.helper.ts (1)
2-5: Type imports from dto/ LGTM.*Aligns helpers with the DTO move.
src/helpers/search.helper.ts (1)
14-35: Repo‑wide: scan for optional‑chaining → method‑call hazardsOptional chaining that only guards a property but not the subsequent method call (e.g., obj?.prop.replace(...)) can still call a method on undefined — audit and guard the method target (use ?. before the call, provide defaults, or assert presence).
Affected: src/helpers/search.helper.ts — lines 14–35 (examples: getSearchYear uses [0]?.innerText.replace(...); getSearchColorRating uses .classNames.split(...); getSearchTitle/getSearchUrl/getSearchPoster call properties on querySelector results without null-checks).
Run locally to find occurrences:
rg -nP --hidden -g '!.git' -g '!node_modules' -g '/.{ts,tsx,js,jsx}' '?.\s[A-Za-z0-9_$]+.[A-Za-z0-9_$]+\s*(' -C2
rg -nP --hidden -g '/.{ts,tsx,js,jsx}' '?.\sinnerText.replace(' -C2
rg -nP --hidden -g '/.{ts,tsx,js,jsx}' '?.\sclassNames.split(' -C2
rg -nP --hidden -g '/.{ts,tsx,js,jsx}' '?.\ssplit([^)]*).map(' -C2Quick fixes: use expr?.prop?.replace(...), (expr ?? '').replace(...), or check for existence before calling.
src/helpers/cinema.helper.ts (2)
79-79: Callsite OK after color-rating fixWith
getCinemaColorRating(el: HTMLElement | null), this line becomes null-safe. No further changes needed.Ensure
.name .iconexists in all tested pages; otherwise expect'unknown'ratings from the fallback.
66-66: Keep the signature; guard for missing filmsgetCinemaFilms is called externally (tests/cinema.helper.test.ts:76 and src/helpers/cinema.helper.ts:66) — do not remove the unused date parameter; change the return to:
return { date: dateText, films: films ? getCinemaFilms('', films) : [] };Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (16)
tsconfig.json (1)
4-4: Resolve missing ES library declarations (unresolved from previous review).The
"lib": ["dom"]setting overrides defaults and excludes ES library types. Without an ES library declaration, TypeScript lacks proper typings for Promise, Array methods, and other standard library features. This was flagged in a previous review and still needs addressing.Align the ES library version with your target (es2015):
- "lib": ["dom"], + "lib": ["es2015", "dom"],src/services/search.service.ts (1)
44-44: parseIdFromUrl can return null but id field expects number.The DTOs (
CSFDSearchMovie) declareidas a non-nullablenumber, butparseIdFromUrlcan returnnull(seesrc/helpers/global.helper.tslines 3-11). This creates a type safety violation.As noted in the previous review, you should guard against null at the call site - either skip creating the entry when
idis null, throw an error, or provide a fallback.Also applies to: 77-77
src/helpers/user-ratings.helper.ts (4)
6-9: Unsafe querySelector access can throw at runtime.As noted in the previous review,
querySelectorcan returnnull, and accessing.attributes.hrefwithout null-checking will throw. Guard with optional chaining and handle the missing case appropriately.
24-26: Unsafe querySelector access can throw at runtime.As noted in the previous review, accessing
.texton a potentially null querySelector result will throw. Guard the call and return a safe fallback (e.g., empty string).
32-35: Type error: classNames is an array, not a string.As noted in the previous review, in node-html-parser,
classNamesis astring[], not a string. Calling.split(' ')will throw. Additionally, the querySelector result needs null-guarding. Access the last element directly from the array instead.
41-44: Unsafe querySelector access can throw at runtime.As noted in the previous review, accessing
.attributes.hrefwithout null-checking the querySelector result will throw when the element is missing. Guard with optional chaining and handle the missing case.src/helpers/cinema.helper.ts (2)
11-15: Fix: classNames is already an array—don't call split.
HTMLElement.classNamesis astring[]in node-html-parser, so calling.split(' ')on it will throw a runtime error.Apply this diff:
-export const getCinemaColorRating = (el: HTMLElement | null): CSFDColorRating => { - const classes: string[] = el?.classNames.split(' ') ?? []; - const last = classes[classes.length - 1] as Colors | undefined; - return last ? parseColor(last as Colors) : 'unknown'; -}; +export const getCinemaColorRating = (el: HTMLElement | null): CSFDColorRating => { + const classes = el?.classNames ?? []; + const last = classes.length ? classes[classes.length - 1] : undefined; + return last ? parseColor(last as Colors) : 'unknown'; +};
72-73: Add null guard before querySelectorAll.
el.querySelectorAllat line 73 will throw whenelis null.Apply this diff:
export const getCinemaFilms = (date: string, el: HTMLElement | null): CSFDCinemaMovie[] => { + if (!el) return []; const filmNodes = el.querySelectorAll('.cinema-table tr');src/helpers/creator.helper.ts (2)
7-11: Fix: classNames is already an array—don't call split.
HTMLElement.classNamesis astring[]in node-html-parser, so calling.split(' ')on it will throw a runtime error.Apply this diff:
-const getCreatorColorRating = (el: HTMLElement | null): CSFDColorRating => { - const classes: string[] = el?.classNames.split(' ') ?? []; - const last = classes[classes.length - 1] as Colors | undefined; - return parseColor(last); -}; +const getCreatorColorRating = (el: HTMLElement | null): CSFDColorRating => { + const classes = el?.classNames ?? []; + const last = classes.length ? classes[classes.length - 1] : undefined; + return last ? parseColor(last as Colors) : 'unknown'; +};
45-49: Fix: guard against undefined before calling split.Line 47:
p?.text?.trim()can beundefined, and calling.split()onundefinedwill throw a runtime error.Apply this diff:
export const getCreatorBio = (el: HTMLElement | null): string | null => { const p = el?.querySelector('.article-content p'); - const first = p?.text?.trim().split('\n')[0]?.trim(); - return first || null; + const text = p?.text?.trim(); + const first = text ? text.split('\n')[0]?.trim() : null; + return first || null; };src/helpers/movie.helper.ts (6)
16-19: Fix: null-safety for querySelector before accessing attributes.Line 17:
querySelector()can returnnull, and accessing.attributes.hrefonnullwill throw a runtime error.Apply this diff:
export const getMovieId = (el: HTMLElement): number => { - const url = el.querySelector('.tabs .tab-nav-list a').attributes.href; + const anchor = el.querySelector('.tabs .tab-nav-list a'); + const url = anchor?.attributes?.href ?? ''; return parseIdFromUrl(url); };
21-23: Fix: null-safety for querySelector before accessing innerText.Line 22:
querySelector('h1')can returnnull, and accessing.innerTextonnullwill throw a runtime error.Apply this diff:
export const getMovieTitle = (el: HTMLElement): string => { - return el.querySelector('h1').innerText.split(`(`)[0].trim(); + const h1 = el.querySelector('h1'); + return h1?.innerText?.split(`(`)[0]?.trim() ?? ''; };
25-28: Fix: null-safety for querySelector before accessing textContent.Line 26:
querySelector('.genres')can returnnull, and accessing.textContentonnullwill throw a runtime error.Apply this diff:
export const getMovieGenres = (el: HTMLElement): CSFDGenres[] => { - const genresRaw = el.querySelector('.genres').textContent; + const genresRaw = el.querySelector('.genres')?.textContent ?? ''; return genresRaw.split(' / ') as CSFDGenres[]; };
30-34: Fix: null-safety for querySelector before accessing textContent.Line 31:
querySelector('.origin')can returnnull, and accessing.textContentonnullwill throw a runtime error.Apply this diff:
export const getMovieOrigins = (el: HTMLElement): string[] => { - const originsRaw = el.querySelector('.origin').textContent; + const originsRaw = el.querySelector('.origin')?.textContent ?? ''; const origins = originsRaw.split(',')[0]; return origins.split(' / '); };
211-215: Fix: null-safety in getBoxContent for nested querySelector.Line 213:
querySelector('h3')can returnnull, and accessing.textContentonnullwill throw a runtime error.Apply this diff:
const getBoxContent = (el: HTMLElement, box: string): HTMLElement => { const headers = el.querySelectorAll('section.box .box-header'); - return headers.find((header) => header.querySelector('h3').textContent.trim().includes(box)) - ?.parentNode; + return headers.find((header) => { + const h3 = header.querySelector('h3'); + return h3?.textContent?.trim()?.includes(box) ?? false; + })?.parentNode; };
180-188: Fix: null-safety for textContent before calling trim.Line 182:
textContentcan benull, and calling.trim()onnullwill throw a runtime error.Apply this diff:
export const getMovieGroup = (el: HTMLElement, group: CSFDCreatorGroups): CSFDMovieCreator[] => { const creators = el.querySelectorAll('.creators h4'); - const element = creators.filter((elem) => elem.textContent.trim().includes(group))[0]; + const element = creators.filter((elem) => elem.textContent?.trim()?.includes(group))[0]; if (element?.parentNode) { return parseMoviePeople(element.parentNode as HTMLElement); } else { return []; } };
🧹 Nitpick comments (5)
examples/cjs/server.ts (1)
6-40: Consider simplifying error handling.The outer
try-catchblock only catches synchronous errors, but the promise chain already has a.catch()handler for asynchronous errors. For example code clarity, you could remove the outertry-catchsince the initialization is unlikely to throw.Optionally simplify to:
csfd .movie(2) .then((res: CSFDMovie) => { const html = `...`; fs.writeFileSync('index.html', html); console.log(`${TYPE}: ✅ index.html has been created with title: ${res.title}`); }) .catch((err) => console.error(`${TYPE}: ❌ Error:`, err));examples/cjs/server.js (1)
6-40: Consider simplifying error handling.The outer
try-catchblock only catches synchronous errors, but the promise chain already has a.catch()handler for asynchronous errors. For example code clarity, you could remove the outertry-catchsince the initialization is unlikely to throw.Optionally simplify to:
csfdLib.csfd .movie(2) .then((res) => { const html = `...`; fs.writeFileSync('index.html', html); console.log(`${TYPE}: ✅ index.html has been created with title: ${res.title}`); }) .catch((err) => console.error(`${TYPE}: ❌ Error:`, err));examples/esm/server.ts (1)
22-23: Consider adding a fallback for descriptions array access.The code accesses
res.descriptions[0]without checking if the array is non-empty. If descriptions is empty, this will render "undefined" in the HTML.Apply this diff to add a fallback:
- <p>${res.descriptions[0]}</p> + <p>${res.descriptions[0] || 'No description available'}</p>package.json (1)
92-101: Add "types" condition to the exports map.The exports map is missing the "types" condition, which should be placed first for proper TypeScript resolution. Modern TypeScript/Node toolchains prefer the conditional "exports.types" over the top-level "types" field when an exports map is present.
Apply this diff to add the "types" condition:
"exports": { ".": { + "types": "./dist/index.d.ts", "import": "./dist/index.mjs", "require": "./dist/index.js" },Keep the top-level
"types": "./dist/index.d.ts"as a fallback for older toolchains.src/services/creator.service.ts (1)
29-35: Consider caching getCreatorBirthdayInfo result.
getCreatorBirthdayInfois called three times to extractbirthday,birthPlace, andage. Since it returns an object containing all three properties, you can optimize by calling it once and destructuring.Apply this diff:
private buildCreator(id: number, asideEl: HTMLElement, filmsNode: HTMLElement) { + const birthdayInfo = getCreatorBirthdayInfo(asideEl); this.person = { id, name: getCreatorName(asideEl), - birthday: getCreatorBirthdayInfo(asideEl)?.birthday, - birthplace: getCreatorBirthdayInfo(asideEl)?.birthPlace, + birthday: birthdayInfo?.birthday, + birthplace: birthdayInfo?.birthPlace, photo: getCreatorPhoto(asideEl), - age: getCreatorBirthdayInfo(asideEl)?.age || null, + age: birthdayInfo?.age || null, bio: getCreatorBio(asideEl), films: getCreatorFilms(filmsNode) }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
examples/cjs/yarn.lockis excluded by!**/yarn.lock,!**/*.lockexamples/esm/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (39)
LICENSE(1 hunks)examples/.gitignore(1 hunks)examples/cjs/package.json(1 hunks)examples/cjs/server.js(1 hunks)examples/cjs/server.ts(1 hunks)examples/esm/package.json(1 hunks)examples/esm/server.js(1 hunks)examples/esm/server.ts(1 hunks)package-json-fix.rolldown.ts(1 hunks)package.json(3 hunks)server.ts(1 hunks)src/dto/cinema.ts(1 hunks)src/dto/index.ts(1 hunks)src/dto/movie.ts(1 hunks)src/dto/search.ts(2 hunks)src/helpers/cinema.helper.ts(2 hunks)src/helpers/creator.helper.ts(2 hunks)src/helpers/global.helper.ts(1 hunks)src/helpers/movie.helper.ts(13 hunks)src/helpers/search.helper.ts(1 hunks)src/helpers/user-ratings.helper.ts(2 hunks)src/index.ts(2 hunks)src/services/cinema.service.ts(2 hunks)src/services/creator.service.ts(2 hunks)src/services/movie.service.ts(2 hunks)src/services/search.service.ts(3 hunks)src/services/user-ratings.service.ts(3 hunks)src/types.ts(1 hunks)src/vars.ts(1 hunks)tests/cinema.helper.test.ts(5 hunks)tests/cinema.service.test.ts(1 hunks)tests/creator.test.ts(7 hunks)tests/fetchers.test.ts(4 hunks)tests/movie.test.ts(13 hunks)tests/search.test.ts(10 hunks)tests/services.test.ts(1 hunks)tests/user-ratings.test.ts(2 hunks)tsconfig.json(1 hunks)tsdown.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- LICENSE
- examples/.gitignore
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/cinema.service.test.ts
- src/helpers/global.helper.ts
- server.ts
- src/types.ts
- tests/creator.test.ts
- src/vars.ts
- src/helpers/search.helper.ts
- src/services/cinema.service.ts
🧰 Additional context used
🧬 Code graph analysis (19)
src/dto/search.ts (1)
src/dto/movie.ts (1)
CSFDMovieCreator(70-81)
examples/cjs/server.ts (4)
examples/cjs/server.js (4)
TYPE(4-4)result(7-7)html(13-32)fs(1-1)examples/esm/server.js (3)
TYPE(4-4)res(7-7)html(9-28)src/index.ts (1)
csfd(51-57)src/dto/movie.ts (1)
CSFDMovie(3-20)
src/services/creator.service.ts (1)
src/helpers/creator.helper.ts (5)
getCreatorName(17-20)getCreatorBirthdayInfo(22-43)getCreatorPhoto(51-54)getCreatorBio(45-49)getCreatorFilms(67-91)
tests/user-ratings.test.ts (2)
src/helpers/user-ratings.helper.ts (7)
getUserRatingId(6-9)getUserRatingType(18-22)getUserRatingTitle(24-26)getUserRatingYear(28-30)getUserRatingColorRating(32-35)getUserRatingDate(37-39)getUserRatingUrl(41-44)src/dto/global.ts (2)
CSFDFilmTypes(25-36)CSFDColorRating(21-21)
src/helpers/user-ratings.helper.ts (3)
src/dto/global.ts (2)
CSFDFilmTypes(25-36)CSFDColorRating(21-21)src/helpers/global.helper.ts (1)
parseColor(29-42)src/dto/user-ratings.ts (1)
Colors(21-21)
src/services/search.service.ts (3)
src/helpers/search.helper.ts (8)
getSearchUrl(22-24)getSearchTitle(14-16)getSearchYear(18-20)getSearchType(9-12)getSearchColorRating(26-30)getSearchPoster(32-35)getSearchOrigins(37-42)parseSearchPeople(44-66)src/dto/search.ts (1)
CSFDSearchMovie(11-15)src/helpers/global.helper.ts (1)
parseIdFromUrl(4-12)
tests/cinema.helper.test.ts (1)
src/helpers/cinema.helper.ts (3)
getCinemaUrlId(22-25)getCinemaCoords(27-45)getCinemaFilms(72-93)
examples/cjs/server.js (1)
examples/esm/server.js (2)
TYPE(4-4)res(7-7)
tsdown.config.ts (1)
package-json-fix.rolldown.ts (1)
copyAndFixPackageJson(5-36)
src/helpers/creator.helper.ts (4)
src/dto/global.ts (1)
CSFDColorRating(21-21)src/dto/user-ratings.ts (1)
Colors(21-21)src/helpers/global.helper.ts (3)
parseColor(29-42)parseIdFromUrl(4-12)addProtocol(44-46)src/dto/creator.ts (1)
CSFDCreatorScreening(14-14)
src/helpers/cinema.helper.ts (4)
src/dto/global.ts (1)
CSFDColorRating(21-21)src/helpers/global.helper.ts (2)
parseColor(29-42)parseIdFromUrl(4-12)src/dto/user-ratings.ts (1)
Colors(21-21)src/dto/cinema.ts (1)
CSFDCinemaMovie(18-21)
examples/esm/server.js (2)
examples/cjs/server.js (3)
TYPE(4-4)html(13-32)fs(1-1)src/index.ts (1)
csfd(51-57)
examples/esm/server.ts (4)
examples/cjs/server.js (3)
TYPE(4-4)html(13-32)fs(1-1)examples/esm/server.js (3)
TYPE(4-4)res(7-7)html(9-28)src/dto/movie.ts (1)
CSFDMovie(3-20)src/index.ts (1)
csfd(51-57)
src/services/user-ratings.service.ts (2)
src/helpers/user-ratings.helper.ts (7)
getUserRatingType(18-22)getUserRatingId(6-9)getUserRatingTitle(24-26)getUserRatingYear(28-30)getUserRatingUrl(41-44)getUserRatingColorRating(32-35)getUserRatingDate(37-39)src/dto/global.ts (1)
CSFDColorRating(21-21)
tests/fetchers.test.ts (3)
src/index.ts (1)
csfd(51-57)src/services/cinema.service.ts (1)
cinemas(16-29)src/dto/cinema.ts (1)
CSFDCinema(3-11)
tests/search.test.ts (3)
src/helpers/search.helper.ts (8)
getSearchTitle(14-16)getSearchYear(18-20)getSearchUrl(22-24)getSearchType(9-12)getSearchColorRating(26-30)getSearchPoster(32-35)getSearchOrigins(37-42)parseSearchPeople(44-66)src/dto/global.ts (2)
CSFDFilmTypes(25-36)CSFDColorRating(21-21)src/dto/movie.ts (1)
CSFDMovieCreator(70-81)
tests/movie.test.ts (3)
src/helpers/movie.helper.ts (19)
getMovieId(16-19)getMovieTitle(21-23)getMoviePoster(122-137)getMovieRandomPhoto(139-147)getMovieTrivia(149-156)getMovieDuration(72-96)getMovieVods(195-208)getMovieTitlesOther(98-120)getMovieOrigins(30-34)getMovieDescriptions(158-162)getMovieGenres(25-28)getMovieType(190-193)getMovieYear(62-70)getMovieRatingCount(52-60)getMovieRating(40-50)getMovieColorRating(36-38)getMovieGroup(180-188)getMoviePremieres(233-251)getMovieBoxMovies(217-231)src/dto/movie.ts (5)
CSFDVod(47-50)CSFDTitlesOther(65-68)CSFDMovieCreator(70-81)CSFDPremiere(145-150)CSFDMovieListItem(83-87)src/dto/global.ts (1)
CSFDColorRating(21-21)
src/services/movie.service.ts (3)
src/helpers/movie.helper.ts (18)
getMovieTitle(21-23)getMovieYear(62-70)getMovieDuration(72-96)getMovieDescriptions(158-162)getMovieGenres(25-28)getMovieOrigins(30-34)getMovieColorRating(36-38)getMovieRating(40-50)getMovieRatingCount(52-60)getMovieTitlesOther(98-120)getMoviePoster(122-137)getMovieRandomPhoto(139-147)getMovieTrivia(149-156)getMovieGroup(180-188)getMovieVods(195-208)getMovieTags(253-256)getMoviePremieres(233-251)getMovieBoxMovies(217-231)src/dto/global.ts (1)
CSFDFilmTypes(25-36)src/vars.ts (1)
movieUrl(7-8)
src/helpers/movie.helper.ts (3)
src/helpers/global.helper.ts (2)
parseIdFromUrl(4-12)getColor(14-27)src/dto/movie.ts (8)
CSFDGenres(89-131)CSFDTitlesOther(65-68)CSFDMovieCreator(70-81)CSFDCreatorGroups(133-143)CSFDVod(47-50)CSFDBoxContent(152-152)CSFDMovieListItem(83-87)CSFDPremiere(145-150)src/dto/global.ts (1)
CSFDColorRating(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (25)
tsconfig.json (2)
11-12: The review comment is incorrect and can be safely ignored.The codebase contains no
server.tsfile, no JSON imports, and no package.json references. RemovingresolveJsonModuledoes not pose a risk because there is nothing in the codebase that depends on it. The original concern was based on an assumption that no longer holds.Likely an incorrect or invalid review comment.
8-9: The review comment is based on incorrect assumptions; build tooling is already properly configured for dual ESM+CommonJS output.The build pipeline already handles dual output correctly:
- tsdown.config.ts specifies
format: ['esm', 'cjs'], explicitly generating both formats- package.json has conditional exports configured with separate entry points for import (ESM) and require (CommonJS)
- The build script (
tsdown --config-loader unconfig) properly loads this configurationYour concern about whether
"module": "esnext"in tsconfig.json generates both formats was valid in principle—TypeScript alone cannot—but the actual build tooling (tsdown) is already configured to produce both outputs regardless of that setting.Regarding the claimed target downgrade: no references to
es2017exist in the codebase; the current tsconfig.json shows"target": "es2015". This claim cannot be verified from the available code state.Likely an incorrect or invalid review comment.
examples/cjs/package.json (1)
1-18: LGTM!The CommonJS example package configuration is appropriate, with correct module type, sensible build scripts, and proper dependencies for testing CJS compatibility.
src/dto/cinema.ts (1)
1-1: LGTM!The import path update correctly reflects the migration from interface-based modules to DTO-based modules.
tests/services.test.ts (1)
2-2: LGTM!The import path update correctly reflects the migration to DTO-based modules. No behavioral changes.
src/dto/index.ts (1)
1-6: LGTM!The barrel export consolidates all DTO modules into a single entry point, providing a clean import surface for consumers.
examples/esm/server.js (1)
6-34: Clean ESM implementation!Good use of top-level await with appropriate error handling. Unlike the CJS examples, this doesn't have an unused
resultvariable.examples/esm/package.json (1)
1-18: LGTM!The ESM example package configuration is appropriate, with correct module type declaration and scripts that mirror the CJS example for testing ESM compatibility.
src/index.ts (1)
1-58: LGTM! Clean DTO migration.The import path updates and the new barrel export (
export type * from './dto') align well with the DTO-based refactor. This provides a clean, consolidated public API surface for consumers.tests/fetchers.test.ts (3)
3-6: LGTM! DTO imports aligned.The import path updates to DTO sources are consistent with the repository-wide refactor.
172-174: Good addition of input validation test.The test ensures the movie endpoint properly rejects non-number inputs.
231-233: Good addition of input validation test.The test ensures the creator endpoint properly rejects non-number inputs.
tsdown.config.ts (1)
1-17: LGTM! Well-configured dual-format build.The tsdown configuration is appropriate for a library:
- Dual ESM/CJS output with declaration files
- Unbundled format keeps dependencies external
- Clean builds and sourcemaps enabled
- Plugin sanitizes package.json by removing dev-only fields
src/dto/search.ts (1)
1-32: LGTM! Consistent type rename.The transition from
CSFDCreatortoCSFDMovieCreatoris applied consistently across imports, interface extensions, and property types. The more specific name better reflects the type's scope within the movie/search domain.package.json (1)
9-16: LGTM! Build process modernized.The new tsdown-based build flow with example scripts provides a cleaner build pipeline and better demonstrates ESM/CJS compatibility.
src/services/user-ratings.service.ts (1)
1-106: LGTM! Consistent helper rename and DTO migration.The helper function renames (e.g.,
getId→getUserRatingId) improve clarity by making the context explicit, and all call sites have been updated consistently. The import path updates to DTO sources align with the project-wide refactor.package-json-fix.rolldown.ts (1)
5-36: LGTM! Plugin correctly sanitizes dist package.json.The plugin appropriately:
- Reads the source package.json
- Removes outDir prefixes from paths
- Strips dev-only fields
- Writes the cleaned version to the dist directory
tests/search.test.ts (1)
3-15: LGTM! DTO migration and helper renames look clean.The import updates correctly reference the new DTO paths and the renamed "Search" variant helpers. All test invocations throughout the file consistently use the updated function names.
tests/cinema.helper.test.ts (1)
52-127: Excellent edge-case coverage for coordinate parsing!The comprehensive test suite now covers critical edge cases including null elements, missing attributes, non-finite values (Infinity, NaN), negative coordinates, and zero coordinates. This significantly improves the reliability of the
getCinemaCoordshelper.src/services/movie.service.ts (1)
5-25: LGTM! Clean migration to DTO-based helpers.All import paths correctly reference the new DTO modules and the renamed
getMovie*helper variants. The refactor maintains consistency across the service layer.tests/user-ratings.test.ts (1)
120-196: Excellent edge-case coverage for color rating parsing!The new test cases comprehensively validate color rating edge cases including grey/lightgrey colors, unknown/invalid color classes, empty strings, and multiple classes. This significantly improves confidence in the color parsing logic.
src/dto/movie.ts (1)
53-62: LGTM! Clean DTO interface updates.The rename from
CSFDCreatortoCSFDMovieCreatorand the corresponding field type updates inCSFDCreatorsare clean and consistent with the broader DTO migration throughout the PR.Also applies to: 70-81
src/services/search.service.ts (1)
7-15: LGTM! Helper imports correctly updated.All search helper imports properly reference the renamed
getSearch*variants, consistent with the broader DTO migration.src/helpers/user-ratings.helper.ts (1)
46-59: Good encapsulation: parseColor made private.Making
parseColora private helper is a good API design decision, keeping the module's public surface focused on the core user rating extraction functions.tests/movie.test.ts (1)
1-611: LGTM: Test suite aligned with refactored API surface.The test imports and function calls have been systematically updated to reflect the DTO-based module structure and
getMovieXhelper naming. Test expectations remain unchanged, confirming behavior preservation.
| let result; | ||
| csfdLib.csfd | ||
| .movie(2) | ||
| .then((res) => { | ||
| result = res; |
There was a problem hiding this comment.
Remove unused variable.
The result variable is declared and assigned but never used, making it dead code.
Apply this diff to remove it:
try {
- let result;
csfdLib.csfd
.movie(2)
.then((res) => {
- result = res;
-
const html = `📝 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.
| let result; | |
| csfdLib.csfd | |
| .movie(2) | |
| .then((res) => { | |
| result = res; | |
| csfdLib.csfd | |
| .movie(2) | |
| .then((res) => { | |
| const html = ` |
🤖 Prompt for AI Agents
In examples/cjs/server.js around lines 7 to 11, the variable `result` is
declared and assigned from the promise but never used; remove the dead variable
and its assignment by eliminating the `let result;` declaration and the `result
= res;` assignment inside the `.then` callback, and instead use `res` directly
inside the callback or return/forward it as needed.
| </head> | ||
| <body> | ||
| <h1>${res.title}</h1> | ||
| <p>${res.descriptions[0]}</p> |
There was a problem hiding this comment.
Validate array access to prevent potential runtime errors.
The code accesses res.descriptions[0] without checking if the array has elements. If the API returns an empty descriptions array, this will interpolate undefined into the HTML.
Consider adding a fallback:
- <p>${res.descriptions[0]}</p>
+ <p>${res.descriptions[0] || 'No description available'}</p>📝 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.
| <p>${res.descriptions[0]}</p> | |
| <p>${res.descriptions[0] || 'No description available'}</p> |
🤖 Prompt for AI Agents
In examples/cjs/server.js around line 27, the template directly uses
res.descriptions[0] which can throw or render undefined if descriptions is empty
or missing; update the template to safely access the first element (e.g., check
that Array.isArray(res.descriptions) && res.descriptions.length > 0 and use that
value) and provide a fallback string (like "No description available") so the
HTML always receives a defined string.
| let result; | ||
| csfd | ||
| .movie(2) | ||
| .then((res: CSFDMovie) => { | ||
| result = res; |
There was a problem hiding this comment.
Remove unused variable.
The result variable is declared and assigned but never used, making it dead code.
Apply this diff to remove it:
try {
- let result;
csfd
.movie(2)
.then((res: CSFDMovie) => {
- result = res;
-
const html = `Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In examples/cjs/server.ts around lines 7 to 11, the local variable `result` is
declared and later assigned inside the promise handler but never used; remove
the dead code by deleting the `let result;` declaration and the `result = res;`
assignment inside the `.then()` callback, and instead use `res` directly within
the callback (or return/handle it as needed).
| </head> | ||
| <body> | ||
| <h1>${res.title}</h1> | ||
| <p>${res.descriptions[0]}</p> |
There was a problem hiding this comment.
Validate array access to prevent potential runtime errors.
The code accesses res.descriptions[0] without checking if the array has elements. If the API returns an empty descriptions array, this will interpolate undefined into the HTML.
Consider adding a fallback:
- <p>${res.descriptions[0]}</p>
+ <p>${res.descriptions[0] || 'No description available'}</p>📝 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.
| <p>${res.descriptions[0]}</p> | |
| <p>${res.descriptions[0] || 'No description available'}</p> |
🤖 Prompt for AI Agents
In examples/cjs/server.ts around line 27, the code directly accesses
res.descriptions[0] which can be undefined if the descriptions array is empty;
update the template to safely read the first description with a fallback (e.g.,
check Array.isArray(res.descriptions) && res.descriptions.length > 0 ?
res.descriptions[0] : a default string) or use optional chaining with a default
(res.descriptions?.[0] ?? 'No description available') so the HTML never
interpolates undefined.
| </head> | ||
| <body> | ||
| <h1>${res.title}</h1> | ||
| <p>${res.descriptions[0]}</p> |
There was a problem hiding this comment.
Validate array access to prevent potential runtime errors.
The code accesses res.descriptions[0] without checking if the array has elements. If the API returns an empty descriptions array, this will interpolate undefined into the HTML.
Consider adding a fallback:
- <p>${res.descriptions[0]}</p>
+ <p>${res.descriptions[0] || 'No description available'}</p>🤖 Prompt for AI Agents
In examples/esm/server.js around line 23, the template directly uses
res.descriptions[0] which can be undefined if descriptions is empty; update the
template to safely access the first element and provide a fallback (e.g., check
that Array.isArray(res.descriptions) && res.descriptions.length > 0 and use that
value, otherwise use a default string like "No description available") so the
rendered HTML never interpolates undefined.
| function removeOutDir(obj: JsonValue, outDir: string): JsonValue { | ||
| if (typeof obj === 'string') { | ||
| const prefix = `./${outDir}/`; | ||
| if (obj.startsWith(prefix)) { | ||
| // Remove the outDir prefix and normalize the path | ||
| let cleaned = obj.slice(prefix.length); | ||
| cleaned = path.posix.normalize(cleaned); | ||
| // The path must start with ./ if it's relative | ||
| cleaned = cleaned ? `./${cleaned}` : './'; | ||
| return cleaned; | ||
| } | ||
| return obj; | ||
| } |
There was a problem hiding this comment.
Handle edge case in path normalization.
If the input path is exactly "./dist/", after slicing and normalizing you'll get ".", which is truthy, resulting in "./." instead of "./".
Apply this diff to handle the edge case:
let cleaned = obj.slice(prefix.length);
cleaned = path.posix.normalize(cleaned);
// The path must start with ./ if it's relative
- cleaned = cleaned ? `./${cleaned}` : './';
+ cleaned = (cleaned && cleaned !== '.') ? `./${cleaned}` : './';
return cleaned;📝 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.
| function removeOutDir(obj: JsonValue, outDir: string): JsonValue { | |
| if (typeof obj === 'string') { | |
| const prefix = `./${outDir}/`; | |
| if (obj.startsWith(prefix)) { | |
| // Remove the outDir prefix and normalize the path | |
| let cleaned = obj.slice(prefix.length); | |
| cleaned = path.posix.normalize(cleaned); | |
| // The path must start with ./ if it's relative | |
| cleaned = cleaned ? `./${cleaned}` : './'; | |
| return cleaned; | |
| } | |
| return obj; | |
| } | |
| function removeOutDir(obj: JsonValue, outDir: string): JsonValue { | |
| if (typeof obj === 'string') { | |
| const prefix = `./${outDir}/`; | |
| if (obj.startsWith(prefix)) { | |
| // Remove the outDir prefix and normalize the path | |
| let cleaned = obj.slice(prefix.length); | |
| cleaned = path.posix.normalize(cleaned); | |
| // The path must start with ./ if it's relative | |
| cleaned = (cleaned && cleaned !== '.') ? `./${cleaned}` : './'; | |
| return cleaned; | |
| } | |
| return obj; | |
| } |
🤖 Prompt for AI Agents
In package-json-fix.rolldown.ts around lines 43 to 55, the path-normalization
branch produces "./." when the input is exactly "./dist/" because
path.posix.normalize yields "." which is truthy; update the code to detect the
"." result (or an empty string) after normalization and return "./" (or set
cleaned to empty before prefixing) so that exact directory roots become "./"
instead of "./.".
| test('Check screenings', async () => { | ||
| const screenings = cinemas[0].screenings; | ||
| expect(screenings.length).toBeGreaterThan(0); | ||
| }); | ||
| test('Check screenings', async () => { | ||
| const film = cinemas[0].screenings[0].films[0]; | ||
| expect(film.id).toBeDefined(); | ||
| expect(film.meta).toBeDefined(); | ||
| expect(film.title).toBeDefined(); | ||
| expect(film.url).toContain('/film/'); | ||
| }); |
There was a problem hiding this comment.
Fix duplicate test name.
Lines 199 and 203 both use the test name "Check screenings". The second test (line 203) appears to validate film metadata rather than screenings.
Apply this diff to clarify the test name:
- test('Check screenings', async () => {
+ test('Check film metadata', async () => {
const film = cinemas[0].screenings[0].films[0];📝 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.
| test('Check screenings', async () => { | |
| const screenings = cinemas[0].screenings; | |
| expect(screenings.length).toBeGreaterThan(0); | |
| }); | |
| test('Check screenings', async () => { | |
| const film = cinemas[0].screenings[0].films[0]; | |
| expect(film.id).toBeDefined(); | |
| expect(film.meta).toBeDefined(); | |
| expect(film.title).toBeDefined(); | |
| expect(film.url).toContain('/film/'); | |
| }); | |
| test('Check screenings', async () => { | |
| const screenings = cinemas[0].screenings; | |
| expect(screenings.length).toBeGreaterThan(0); | |
| }); | |
| test('Check film metadata', async () => { | |
| const film = cinemas[0].screenings[0].films[0]; | |
| expect(film.id).toBeDefined(); | |
| expect(film.meta).toBeDefined(); | |
| expect(film.title).toBeDefined(); | |
| expect(film.url).toContain('/film/'); | |
| }); |
🤖 Prompt for AI Agents
In tests/fetchers.test.ts around lines 199 to 209, there are two tests sharing
the name "Check screenings" which causes duplicated test names; rename the
second test (the one validating a film's id/meta/title/url around line 203) to a
more accurate name like "Check film metadata" so test names are unique and
reflect the assertions being made.
| "strictNullChecks": false, | ||
| "module": "esnext", | ||
| "moduleResolution": "node", | ||
| "strictNullChecks": false, // will be more strict in future releases |
There was a problem hiding this comment.
Configuration conflict: strictNullChecks: false overrides strict: true.
Line 10 explicitly sets "strictNullChecks": false, which contradicts line 15's "strict": true. The strict mode implicitly enables strictNullChecks, but the explicit false overrides it. Clarify the intent: either remove the explicit false to inherit strict mode's null-checking behavior, or revise the strict setting.
If you genuinely need relaxed null-checking, document and justify this exception:
- "strictNullChecks": false, // will be more strict in future releases
+ // Inherits strict: true; remove if null-checking should remain strict📝 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.
| "strictNullChecks": false, // will be more strict in future releases | |
| // Inherits strict: true; remove if null-checking should remain strict |
🤖 Prompt for AI Agents
In tsconfig.json around line 10, the explicit "strictNullChecks": false
conflicts with "strict": true (line 15) because strict enables strictNullChecks;
remove the explicit "strictNullChecks": false so strict mode's null-checking
applies, or if you truly need relaxed null-checks, set "strict": false and add a
comment documenting and justifying the exception (including potential risks), or
keep "strict": true and change the explicit setting to true for clarity.
ESM + CommonJS building
[Needs more testing]
Summary by CodeRabbit
New Features
Refactor
Chores