ESM + CommonJS build 🎉 #18
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.69% 100.00% +1.30%
===========================================
Files 26 26
Lines 536 541 +5
Branches 104 114 +10
===========================================
+ Hits 529 541 +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. WalkthroughMoved type declarations from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Csfd as Csfd (src/index.ts)
participant Search as SearchScraper
participant HTTP as HTTP Fetch
participant DOM as DOM Parser
participant H as Search Helpers
Client->>Csfd: search(text)
Csfd->>Search: search(text)
Search->>HTTP: GET /hledat?q=text
HTTP-->>Search: HTML
Search->>DOM: parse(HTML)
loop per result
Search->>H: getSearchUrl/Title/Year/Type/Poster/Origins/ColorRating
H-->>Search: fields
Search->>H: parseSearchPeople(directors/actors)
H-->>Search: CSFDMovieCreator[]
end
Search-->>Csfd: CSFDSearch
Csfd-->>Client: CSFDSearch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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.
| const url = getSearchUrl(m); | ||
|
|
||
| const movie: CSFDSearchMovie = { | ||
| id: parseIdFromUrl(url), | ||
| title: getTitle(m), | ||
| year: getYear(m), | ||
| title: getSearchTitle(m), | ||
| year: getSearchYear(m), | ||
| url: `https://www.csfd.cz${url}`, | ||
| type: getType(m), | ||
| colorRating: getColorRating(m), | ||
| poster: getPoster(m), | ||
| origins: getOrigins(m), | ||
| type: getSearchType(m), | ||
| colorRating: getSearchColorRating(m), | ||
| poster: getSearchPoster(m), | ||
| origins: getSearchOrigins(m), | ||
| creators: { | ||
| directors: parsePeople(m, 'directors'), | ||
| actors: parsePeople(m, 'actors') | ||
| directors: parseSearchPeople(m, 'directors'), | ||
| actors: parseSearchPeople(m, 'actors') | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Double-check parseIdFromUrl return type
If parseIdFromUrl can return null, assigning to id: number violates the DTO. Either refine parseIdFromUrl to return number, guard against null, or widen the DTO if needed.
Run to verify the function signature and null returns:
🏁 Script executed:
#!/bin/bash
rg -n -C2 "export const parseIdFromUrl" --type ts
rg -n "\bparseIdFromUrl\s*\(" --type ts | rg -n -v "!==|!=|??|\|\|" -nLength of output: 393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- src/helpers/global.helper.ts ----"
nl -ba src/helpers/global.helper.ts | sed -n '1,240p' || true
echo
echo "---- src/services/search.service.ts (first 120 lines) ----"
nl -ba src/services/search.service.ts | sed -n '1,160p' || true
echo
echo "---- locate CSFDSearchMovie definition ----"
rg -n "CSFDSearchMovie" --hidden --glob '!**/node_modules/**' || trueLength of output: 374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "---- src/helpers/global.helper.ts (up to 300 lines) ----"
if [ -f src/helpers/global.helper.ts ]; then
sed -n '1,300p' src/helpers/global.helper.ts || true
else
echo "MISSING: src/helpers/global.helper.ts"
fi
echo
echo "---- src/services/search.service.ts (up to 300 lines) ----"
if [ -f src/services/search.service.ts ]; then
sed -n '1,300p' src/services/search.service.ts || true
else
echo "MISSING: src/services/search.service.ts"
fi
echo
echo "---- Search for CSFDSearchMovie type/interface ----"
rg -n --hidden -uu "CSFDSearchMovie" || trueLength of output: 5549
parseIdFromUrl returns null but DTOs expect a non-nullable id
src/helpers/global.helper.ts shows parseIdFromUrl returns null in some branches (returns +id || null / return null), while the search DTOs (src/dto/global.ts → CSFDScreening.id and src/dto/search.ts → CSFDSearchMovie/CSFDSearchUser) declare id as number. Fix one of the following:
- Change parseIdFromUrl to a safe non-null number (never return null) and keep DTOs as-is.
- OR change parseIdFromUrl's signature to return number | null and update the DTOs (CSFDScreening / CSFDSearchMovie / CSFDSearchUser) to accept number | null.
- OR guard at call sites (src/services/search.service.ts — the id assignments around lines ~41–55) before assigning/including items (skip, throw, or provide a fallback).
[src/helpers/global.helper.ts, src/services/search.service.ts, src/dto/global.ts, src/dto/search.ts]
🤖 Prompt for AI Agents
In src/services/search.service.ts around lines 41–55, parseIdFromUrl can return
null while CSFDSearchMovie.id is declared as a non-nullable number; fix by
guarding at the call site: call parseIdFromUrl, check if the returned id is
null, and if so skip creating/including that movie entry (or alternatively throw
or provide a defined fallback id), otherwise construct the CSFDSearchMovie with
the non-null id; this keeps DTOs unchanged and prevents assigning null to a
non-nullable field.
| "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.
Pull Request Overview
This PR adds dual ESM and CommonJS build support while refactoring the codebase to use prefixed helper functions and restructured DTO modules for better organization and type safety.
- Adds dual ESM/CommonJS distribution with separate TypeScript configurations
- Refactors helper functions to use domain-specific prefixes (Movie, Creator, Search, UserRating, Cinema)
- Moves interfaces to DTO modules and creates centralized type exports
Reviewed Changes
Copilot reviewed 30 out of 35 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.esm.json, tsconfig.cjs.json | New TypeScript configurations for ESM and CJS builds |
| package.json | Updates build scripts and export configurations for dual module support |
| src/types.ts | New centralized type exports barrel file |
| barrels.json | Configuration for generating barrel exports |
| src/helpers/*.ts | Renamed helper functions with domain prefixes |
| src/dto/*.ts | Restructured type imports from interfaces to DTO modules |
| tests/*.ts | Updated test imports and function calls to match new naming |
| src/services/*.ts | Updated to use new helper function names and DTO imports |
Comments suppressed due to low confidence (1)
tests/cinema.helper.test.ts:1
- Import path is incorrect. The file should import from '../src/dto/cinema' not '../src/dto/cinema.interface' since the file was renamed from cinema.interface.ts to cinema.ts.
import { HTMLElement, parse } from 'node-html-parser';
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
package.json (2)
9-11: Build order: verify barrels target (src vs dist).Barrels run after all tsc builds. If barrels generate sources (src), run them before tsc; if they generate dist barrels, keep as-is. Please confirm the barrels.json target to avoid stale declarations.
17-17: Harden fix-paths step.If npm-prepare-dist ever skips/copies a different package.json path, this command will silently do nothing. Consider guarding with an existence check and failing fast, or extend it to also ensure the "exports" map matches the root.
📜 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 (1)
package.json(3 hunks)
⏰ 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 (2)
package.json (2)
38-38: ESLint 9 + @typescript-eslint 8: confirm peer compatibility.This combo should work, but eslint-config-google@0.14.0 may lag behind ESLint 9 in some setups. Please verify locally that there are no peer warnings and lint runs clean.
Also applies to: 40-42, 45-45, 47-47, 54-56
91-104: Fix conditional exports for types: move "types" to the top-level of "." and flatten import/require.Types under import/require are ignored by TS/Node resolution. Put "types" as the first condition at the "." level; keep top-level "types" as fallback.
Apply:
- "exports": { - ".": { - "import": { - "types": "./index.dto.d.ts", - "default": "./esm/index.js" - }, - "require": { - "types": "./index.dto.d.ts", - "default": "./cjs/index.js" - } - } - } + "exports": { + ".": { + "types": "./index.dto.d.ts", + "import": "./esm/index.js", + "require": "./cjs/index.js" + } + }Optional: also expose package.json for tooling:
"exports": { ".": { "types": "./index.dto.d.ts", "import": "./esm/index.js", "require": "./cjs/index.js" - } + }, + "./package.json": "./package.json" }
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/fetchers.test.ts (1)
1-5: Import types from the barrel to decouple tests from internal layoutUse the public types barrel (
src/types.ts) instead of deepdto/*paths to reduce churn if files move.Please confirm
src/types.tsre‑exports these symbols.-import { CSFDCinema } from '../src/dto/cinema'; -import { CSFDCreatorScreening } from '../src/dto/creator'; -import { CSFDColorRating, CSFDFilmTypes } from '../src/dto/global'; +import { + CSFDCinema, + CSFDCreatorScreening, + CSFDColorRating, + CSFDFilmTypes +} from '../src/types';package.json (2)
54-55: Minor: lint-staged + ESLint CLI OK. Consider using--max-warnings=0in CI.Non-blocking.
16-16: Tidy: call Vitest directly.Avoid "yarn in yarn". Use Vitest directly for coverage.
- "test:coverage": "yarn test run --coverage", + "test:coverage": "vitest run --coverage",
📜 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 (4)
package.json(3 hunks)tests/cinema.service.test.ts(1 hunks)tests/creator.test.ts(7 hunks)tests/fetchers.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/creator.test.ts
- tests/cinema.service.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
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)
⏰ 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 (8)
tests/fetchers.test.ts (3)
56-58: Negative test looks good; confirm error type is stableIf the implementation throws a specific error (e.g., TypeError or custom), consider asserting that type/message for stronger guarantees.
115-117: Negative test looks good; confirm error type is stableSame note as for movie: if a specific error class/message is thrown, assert it for stronger guarantees.
73-99: Harden cinema tests: avoid brittle indexing, fix comment/title, add timeoutGuard against empty/short responses, remove duplicate test title, and extend beforeAll timeout for live fetch.
File: tests/fetchers.test.ts (around lines 73–99)-// Search +// Cinemas describe('Live: Cinemas', () => { let cinemas: CSFDCinema[] - beforeAll(async () => { - cinemas = await csfd.cinema(1, 'today'); - }); + beforeAll(async () => { + cinemas = await csfd.cinema(1, 'today'); + }, 30000); test('Check city', async () => { + expect(cinemas.length).toBeGreaterThan(0); const pragueCinemas = cinemas.filter((x) => x.city.includes('Praha')); expect(pragueCinemas.length).toBeGreaterThan(0); }); test('Check screenings', async () => { - const screenings = cinemas[0].screenings; + expect(cinemas[0]).toBeDefined(); + const screenings = cinemas[0].screenings; expect(screenings.length).toBeGreaterThan(0); }); - test('Check screenings', async () => { - const film = cinemas[0].screenings[0].films[0]; + test('Check film details', async () => { + expect(cinemas[0]?.screenings?.[0]?.films?.[0]).toBeDefined(); + 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 showtimes', async () => { - const film = cinemas[0].screenings[0].films[1].showTimes[0]; - expect(film).toBeDefined(); + expect(cinemas[0]?.screenings?.[0]?.films?.[0]).toBeDefined(); + const showtime = cinemas[0].screenings[0].films[0].showTimes?.[0]; + expect(showtime).toBeDefined(); }); });Search didn't find a Csfd class in the repo — confirm csfd.cinema(...) is the correct API.
package.json (5)
9-11: Run barrels before compilation and remove redundant tsc invocation.Barrel generation should precede tsc so generated indexes are compiled. Also, the initial bare
tscis likely redundant with the two targeted builds.[Suggest_recommended_refactor]
- "build": "tsc && tsc -p tsconfig.cjs.json && tsc -p tsconfig.esm.json && yarn barrels", + "build": "yarn barrels && tsc -p tsconfig.cjs.json && tsc -p tsconfig.esm.json",
45-45: Barrelsby in devDeps is fine. Ensure barrels.json is excluded from publish.Confirm barrels.json isn’t included in dist tarball (you publish from dist, so likely fine).
47-47: ESLint v9 chosen. Ensure config migrated to flat config.If you still use legacy extends (e.g., eslint-config-google), expect breakage on v9.
91-104: Fix invalid export map: move "types" to top-level conditions and avoid nesting under "import"/"require".Current structure nests "types" inside "import"/"require", which is not a valid conditional-exports shape. Place "types" as a sibling of "import"/"require" (ideally first), and optionally add a default and a subpath for package.json.
Apply:
- "exports": { - ".": { - "import": { - "types": "./index.dto.d.ts", - "default": "./esm/index.js" - }, - "require": { - "types": "./index.dto.d.ts", - "default": "./cjs/index.js" - } - } - } + "exports": { + ".": { + "types": "./index.dto.d.ts", + "import": "./esm/index.js", + "require": "./cjs/index.js", + "default": "./esm/index.js" + }, + "./package.json": "./package.json" + }
38-43: Confirm ESLint mode or migrate eslint-config-googleeslint-config-google is an eslintrc-style (legacy) shareable config and does not provide a native ESLint v9 flat-config export — if your project runs ESLint in flat mode you will get lint failures; either enable eslintrc compatibility (ESLINT_USE_FLAT_CONFIG=false or use LegacyESLint) or migrate the config (e.g., @eslint/migrate-config) / replace with a flat-native config.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/helpers/creator.helper.ts (1)
22-43: Fix: null-safety and return-type in getCreatorBirthdayInfo
el.querySelectorand.trim()calls can NPE; add?..agecurrently can benullbut the return type declaresnumber; also unary+onnullyields0unexpectedly. Returnnumber | nulland drop the coercion.-export const getCreatorBirthdayInfo = ( - el: HTMLElement | null -): { birthday: string; age: number; birthPlace: string } => { - const infoBlock = el.querySelector('h1 + p'); - const text = infoBlock?.innerHTML.trim(); +export const getCreatorBirthdayInfo = ( + el: HTMLElement | null +): { birthday: string; age: number | null; birthPlace: string } => { + 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(); + const birthPlaceRow = infoBlock?.querySelector('.info-place')?.innerHTML?.trim(); + const ageRow = infoBlock?.querySelector('.info')?.innerHTML?.trim(); @@ - const age = ageRow ? +parseAge(ageRow) : null; + const age = ageRow ? parseAge(ageRow) : null;src/helpers/cinema.helper.ts (1)
27-45: Harden coords parsing; guard undefined href and parseqrobustly
linkMapsEl.getAttribute('href')can be null;.split('q=')on null will throw, and the current approach breaks when extra query params are present. Parse viaURLSearchParamswith a safe fallback.Apply:
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 a = el.querySelector('a[href*="q="]'); + if (!a) return null; + + const href = a.getAttribute('href') || ''; + + // Try robust parsing first + let q: string | null = null; + try { + const u = new URL(href, 'https://example.com'); + q = u.searchParams.get('q'); + } catch { + // ignore, fallback below + } + // Fallback: split manually and strip trailing params/fragments + if (!q) { + const afterQ = href.split('q=')[1]; + q = afterQ ? afterQ.split(/[&#?]/)[0] : null; + } + if (!q) return null; + + const [latStr, lngStr] = q.split(',', 2); + const lat = Number(latStr); + const lng = Number(lngStr); + return Number.isFinite(lat) && Number.isFinite(lng) ? { lat, lng } : null; };
🧹 Nitpick comments (2)
src/helpers/creator.helper.ts (1)
67-90: Minor robustness: film list traversal
- Prefer getAttribute('href') over accessing
attributes.hrefdirectly.- If querySelectorAll returns a NodeList-like in some versions, consider Array.from to ensure
.mapis available.-export const getCreatorFilms = (el: HTMLElement | null): CSFDCreatorScreening[] => { - const filmNodes = el?.querySelectorAll('.box')?.[0]?.querySelectorAll('table tr') ?? []; +export const getCreatorFilms = (el: HTMLElement | null): CSFDCreatorScreening[] => { + const filmNodes = el?.querySelectorAll('.box')?.[0]?.querySelectorAll('table tr') ?? []; @@ - const films = filmNodes.map((filmNode) => { - const id = getCreatorId(filmNode.querySelector('td.name .film-title-name')?.attributes?.href); + const films = (Array.from(filmNodes) as HTMLElement[]).map((filmNode) => { + const link = filmNode.querySelector('td.name .film-title-name'); + const id = getCreatorId(link?.getAttribute('href') ?? undefined);src/helpers/cinema.helper.ts (1)
66-66: Remove unuseddateparam and update call
getCinemaFilmsdoesn’t usedate. Drop it and simplify the call. Also defaultdateTextto a string to satisfy typing.Apply:
- return { date: dateText, films: getCinemaFilms('', films) }; + return { date: dateText ?? '', films: getCinemaFilms(films) };And change the signature:
-export const getCinemaFilms = (date: string, el: HTMLElement | null): CSFDCinemaMovie[] => { +export const getCinemaFilms = (el: HTMLElement | null): CSFDCinemaMovie[] => {Also applies to: 72-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/helpers/cinema.helper.ts(2 hunks)src/helpers/creator.helper.ts(2 hunks)src/helpers/search.helper.ts(1 hunks)tsconfig.esm.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tsconfig.esm.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/helpers/search.helper.ts
🧰 Additional context used
🧬 Code graph analysis (2)
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)
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)
⏰ 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 (9)
src/helpers/creator.helper.ts (6)
2-4: DTO imports look goodAligned with the new DTO locations and types.
13-15: Type consistency with parseIdFromUrlThis signature is correct here, but
parseIdFromUrl(global.helper.ts) returnsnullper snippet while typed asnumber. Update that function’s return type tonumber | nullto avoid downstream casts and mismatches.Outside this file, adjust:
- export const parseIdFromUrl = (url: string): number => { + export const parseIdFromUrl = (url: string): number | null => {
17-20: Safe name extractionOptional chaining and nullable return are appropriate here.
51-54: LGTM: photo extraction guardedNull-safe and uses addProtocol correctly.
56-65: LGTM: tightened parse helpersConcrete return types and safer parsing are good improvements.
7-10: Fix: null-safety and classNames handling in getCreatorColorRating
el?.classNames.split(' ')will throw (classNames is already an array in node-html-parser), andparseColor(last)can getundefined. Add a safe fallback.-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[classes.length - 1] as Colors | undefined; + return parseColor(last ?? 'lightgrey'); +};src/helpers/cinema.helper.ts (3)
6-9: LGTM: DTO/domain importsImports align with the DTO split and color/type usage.
77-90: Don’t assignnullto requiredid; filter out invalid entries
CSFDMovieListItem.idis a number. Assigningnullbreaks typing; skip entries without a valid URL/ID instead.Apply:
- const films = filmNodes.map((filmNode) => { - const url = filmNode.querySelector('td.name h3 a')?.attributes.href; - const id = url ? getCinemaUrlId(url) : null; - const title = filmNode.querySelector('.name h3')?.text.trim(); - 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) - }; - }); + const films = filmNodes + .map((filmNode) => { + const url = filmNode.querySelector('td.name h3 a')?.attributes.href; + const id = url ? getCinemaUrlId(url) : null; + if (id == null) return null; + const title = filmNode.querySelector('.name h3')?.text.trim() ?? ''; + 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: url!, + colorRating, + showTimes, + meta: parseMeta(meta) + }; + }) + .filter(Boolean) as CSFDCinemaMovie[];
11-14: Fix: null‑safe class handling; remove invalid.spliton string[]
HTMLElement.classNamesisstring[]; current code calls.spliton it and will also throw whenelis null.Apply:
-export const getCinemaColorRating = (el: HTMLElement | null): CSFDColorRating => { - const classes: string[] = el?.classNames.split(' ') ?? []; - const last = classes.length ? classes[classes.length - 1] : 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'; +};
| export const getCinemaUrlId = (url: string | null | undefined): number | null => { | ||
| if (!url) return null; | ||
| return parseIdFromUrl(url); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Align parseIdFromUrl’s type: it returns null in some paths
This wrapper returns number | null, but parseIdFromUrl is declared as number while returning null in its implementation. Fix the source to avoid unsound typing.
Update src/helpers/global.helper.ts:
-export const parseIdFromUrl = (url: string): number => {
+export const parseIdFromUrl = (url: string): number | null => {
if (url) {
const idSlug = url?.split('/')[2];
const id = idSlug?.split('-')[0];
return +id || null;
} else {
return null;
}
};Run to confirm all signatures/usages are consistent:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Declarations =="
rg -nP -C2 'export\s+const\s+parseIdFromUrl\s*=\s*\(url:\s*string\)\s*:\s*number\b'
echo
echo "== Call sites (context) =="
rg -nP -C3 '\bparseIdFromUrl\s*\('Length of output: 6013
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== parseIdFromUrl declaration =="
sed -n '1,120p' src/helpers/global.helper.ts
echo
echo "== Call sites with surrounding context (8 lines) =="
rg -nP -C8 '\bparseIdFromUrl\s*\(' || trueLength of output: 14142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locations that return parseIdFromUrl (with context) =="
rg -nP -C4 'return\s+parseIdFromUrl\(' || true
echo
echo "== Functions exporting and returning parseIdFromUrl result (signatures) =="
rg -nP --hidden --no-ignore-vcs 'export\s+const\s+[A-Za-z0-9_]+\s*=\s*\(.*\)\s*:\s*number\b' -n -C1 || true
echo
echo "== DTO/type definitions referencing 'id' fields (searching common DTO names) =="
rg -nP '\bCSFD(Search(Movie|User)|Movie(ListItem)?|SearchMovie|SearchUser|MovieListItem)\b' || true
echo
echo "== File contents for DTOs and found helpers =="
sed -n '1,240p' src/dto/global.ts || true
sed -n '1,240p' src/dto/movie.ts || true
sed -n '1,240p' src/dto/search.ts || true
sed -n '1,240p' src/dto/user-ratings.ts || trueLength of output: 12183
Fix parseIdFromUrl signature and update callers
parseIdFromUrl is declared to return number but its implementation (and tests) return null on some inputs. Change the helper signature to:
export const parseIdFromUrl = (url: string | null | undefined): number | null =>
Then update/guard all call sites that assume a non-null number (either widen their return types to number | null or add runtime checks/assertions):
- src/helpers/global.helper.ts — change signature as above
- Update callers currently typed to return number:
- src/helpers/movie.helper.ts (getMovieId, parseMoviePeople -> CSFDMovieCreator.id, getMovieBoxMovies -> CSFDMovieListItem.id)
- src/helpers/user-ratings.helper.ts (getUserRatingId)
- src/services/search.service.ts (id assignments for CSFDSearchMovie / CSFDSearchUser)
- src/helpers/search.helper.ts (mapping person -> id)
Callers already nullable-aware and OK: src/helpers/creator.helper.ts:getCreatorId, src/helpers/cinema.helper.ts:getCinemaUrlId
🤖 Prompt for AI Agents
In src/helpers/cinema.helper.ts around lines 22 to 25, parseIdFromUrl is
declared to return number but actually can return null; update its signature to
return number | null (export const parseIdFromUrl = (url: string | null |
undefined): number | null =>) and then update all callers to handle nullable
results: change src/helpers/global.helper.ts signature to accept/return number |
null and adjust call sites that assume non-null in src/helpers/movie.helper.ts
(getMovieId, parseMoviePeople -> CSFDMovieCreator.id, getMovieBoxMovies ->
CSFDMovieListItem.id), src/helpers/user-ratings.helper.ts (getUserRatingId),
src/services/search.service.ts (assignments for CSFDSearchMovie /
CSFDSearchUser), and src/helpers/search.helper.ts (person -> id mapping) by
either widening their types to number | null or adding runtime null
checks/assertions before using the id; leave creator.helper.ts and
cinema.helper.ts callers as-is since they already handle nullable ids.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/user-ratings.test.ts (3)
46-75: Prefer skipped tests over commented-out tests.Use
test.skipto keep intent visible and tracked in CI.- // test('TV film', () => { - // const movie = getUserRatingType(movies[18]); - // expect(movie).toEqual<CSFDFilmTypes>('TV film'); - // }); + test.skip('TV film', () => { + const movie = getUserRatingType(movies[18]); + expect(movie).toEqual<CSFDFilmTypes>('TV film'); + }); - // test('Pořad', () => { - // const movie = getUserRatingType(movies[6]); - // expect(movie).toEqual<CSFDFilmTypes>('pořad'); - // }); + test.skip('Pořad', () => { + const movie = getUserRatingType(movies[6]); + expect(movie).toEqual<CSFDFilmTypes>('pořad'); + });
90-100: Consider removing generics fromtoEqualor asserting types withexpectTypeOf.The generics on
toEqualadd noise and may not be enforced. Preferexpect(movie).toEqual(2022)and type checks viaexpectTypeOf(movie).toEqualTypeOf<number>()if needed.Example:
- expect(movie).toEqual<number>(2022); + expect(movie).toEqual(2022);
103-191: Color class detection: add a case where the color class is not the last class (or refactor helper).Current implementation in
getUserRatingColorRatingappears to rely on the last CSS class; real markup can reorder classes. Add a test (skipped until helper is updated) or refactor the helper to scan all classes for a known color.Add this test:
+ test.skip('Valid color not last should still be recognized', () => { + // Color class appears before other classes + const mockElement = parse(` + <tr> + <td class="name"> + <span class="icon red some-other-class another-one"></span> + </td> + </tr> + `); + const result = getUserRatingColorRating(mockElement); + expect(result).toEqual<CSFDColorRating>('good'); + });Optionally, deduplicate mock creation with a tiny helper in this file:
function buildRowWithIcon(classAttr: string): HTMLElement { return parse(`<tr><td class="name"><span class="icon ${classAttr}"></span></td></tr>`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/cinema.helper.test.ts(5 hunks)tests/user-ratings.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cinema.helper.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
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)
⏰ 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 (6)
tests/user-ratings.test.ts (6)
6-13: Renamed helper imports align with the new API surface.Looks consistent with the helper refactor.
35-44: ID parsing tests look good.Covers first and last rows; assertions are clear.
79-85: Title tests are precise and readable.
193-201: Date parsing tests are clear and stable.
204-212: URL tests look correct and assert the absolute URL.
3-3: Use type-only import for DTO typesCSFDColorRating, CSFDFilmTypes and CSFDStars are exported as types from src/dto/global.ts — switch to a type-only import.
File: tests/user-ratings.test.ts
Apply this diff:
-import { CSFDColorRating, CSFDFilmTypes, CSFDStars } from '../src/dto/global'; +import type { CSFDColorRating, CSFDFilmTypes, CSFDStars } from '../src/dto/global';
2 participants
ESM + CommonJS building
[Needs more testing]
Summary by CodeRabbit
New Features
Refactor
Chores
Tests