Conversation
bartholomej
force-pushed
the
tv-series
branch
from
fafd5d6 to
f6c21cc
Compare
|
Here's the code health analysis summary for commits Analysis Summary
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds DTO fields for seasons/episodes/parent/episodeCode/seasonName, a URL-id parser, new DOM-parsing helpers to detect/extract series/seasons/episodes and episode codes, integrates them into the movie service to populate new fields, expands tests and large HTML mocks, and tweaks the demo invocation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant MovieService as MovieService
participant MovieHelpers as MovieHelpers
participant GlobalHelper as GlobalHelper
participant DOM as HTMLElement
Client->>MovieService: buildMovie(el: HTMLElement)
MovieService->>MovieHelpers: getMovieType(el)
MovieHelpers-->>MovieService: type
alt type == "série"
MovieService->>MovieHelpers: getSerieasAndSeasonTitle(el)
MovieHelpers-->>MovieService: {seriesName, seasonName}
MovieService->>MovieHelpers: detectSeasonOrEpisodeListType(el)
MovieHelpers-->>MovieService: listType
alt listType == "seasons" or "episodes"
MovieService->>MovieHelpers: getSeasonsOrEpisodes(el, serie?)
MovieHelpers->>GlobalHelper: parseLastIdFromUrl(url)
GlobalHelper-->>MovieHelpers: id
MovieHelpers-->>MovieService: CSFDSeason[]
MovieService->>MovieService: set seasons/episodes
end
MovieService->>MovieHelpers: getSeasonorEpisodeParent(el, serie?)
MovieHelpers-->>MovieService: CSFDParent | null
MovieService->>MovieService: set parent
end
alt type == "epizoda"
MovieService->>MovieHelpers: getEpisodeCode(el)
MovieHelpers-->>MovieService: episodeCode
MovieService->>MovieService: set episodeCode
end
MovieService-->>Client: CSFDMovie (includes new fields)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #60 +/- ##
==========================================
- Coverage 99.54% 99.46% -0.09%
==========================================
Files 28 33 +5
Lines 662 745 +83
Branches 151 186 +35
==========================================
+ Hits 659 741 +82
- Misses 3 4 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bartholomej
force-pushed
the
tv-series
branch
from
f6c21cc to
f884613
Compare
bartholomej
force-pushed
the
tv-series
branch
from
f884613 to
e9326cd
Compare
bartholomej
force-pushed
the
hybrid
branch
3 times, most recently
from
11636b4 to
a45258d
Compare
bartholomej
force-pushed
the
tv-series
branch
from
e9326cd to
da3c2fe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/dto/movie.ts (1)
166-171: Clarify episode/season child type or keep CSFDSeason strict.If you intend to reuse this for both seasons and episodes, consider:
- Keep CSFDSeason strict (non-null fields) and ensure the parser filters out incomplete items (see helper fix).
- Or add an alias for readability:
export type CSFDEpisode = CSFDSeason;Confirm whether URLs stored here should be absolute; current helpers return relative links.
demo.ts (1)
11-11: Commented creator example is OK.Consider leaving a short note why it’s disabled.
src/services/movie.service.ts (1)
6-8: Rename helper: getSerieasAndSeasonTitle → getSeriesAndSeasonTitle.Fix the spelling in import and usage for clarity and future discoverability.
Apply:
- getSerieasAndSeasonTitle + getSeriesAndSeasonTitleand
-const { seriesName = null, seasonName = null } = type === 'série' ? getSerieasAndSeasonTitle(el) : {}; +const { seriesName = null, seasonName = null } = type === 'série' ? getSeriesAndSeasonTitle(el) : {};Also applies to: 26-30, 61-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
demo.ts(1 hunks)src/dto/movie.ts(3 hunks)src/helpers/global.helper.ts(1 hunks)src/helpers/movie.helper.ts(2 hunks)src/services/movie.service.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/services/movie.service.ts (2)
src/helpers/movie.helper.ts (12)
getSerieasAndSeasonTitle(23-39)detectSeasonOrEpisodeListType(231-237)getMovieTitle(41-43)getMovieYear(82-90)getMovieDescriptions(178-182)getMovieGroup(261-269)getMovieVods(276-289)getMovieTags(334-337)getMovieBoxMovies(298-312)getSeasonsOrEpisodes(200-218)getSeasonorEpisodeParent(239-259)getEpisodeCode(220-229)src/dto/global.ts (1)
CSFDFilmTypes(25-36)
demo.ts (2)
src/index.ts (2)
csfd(51-57)movie(28-30)src/services/movie.service.ts (1)
movie(36-52)
src/helpers/movie.helper.ts (2)
src/helpers/global.helper.ts (2)
parseIdFromUrl(4-12)parseLastIdFromUrl(14-22)src/dto/movie.ts (2)
CSFDSeason(166-171)CSFDParent(27-30)
⏰ 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 (4)
src/dto/movie.ts (1)
20-25: New CSFDMovie fields look good; ensure DTOs match emitted data.The additions are coherent. After fixing helper nullability (see other comments), this shape should be stable.
Please confirm tests cover population of: seasons, episodes, parent, episodeCode, seasonName.
demo.ts (1)
5-5: Demo change is fine.No issues; keep demo noise out of CI runs if applicable.
src/helpers/movie.helper.ts (2)
220-229: Episode code extraction LGTM; consider unit test.Regex-based extraction is fine. Add a simple test to lock behavior with/without parentheses.
231-237: List type detection LGTM; add fallback test.Heuristic is simple and readable. Add tests for pages lacking the header to ensure null is returned.
src/helpers/movie.helper.ts
Outdated
| export const getSerieasAndSeasonTitle = (el: HTMLElement): { seriesName: string; seasonName: string | null } => { | ||
| const titleElement = el.querySelector('h1'); | ||
| if (!titleElement) { | ||
| return { seriesName: null, seasonName: null }; | ||
| } | ||
|
|
||
| const fullText = titleElement.innerText.trim(); | ||
|
|
||
| // Check if there's a series part indicated by ' - ' | ||
| if (fullText.includes(' - ')) { | ||
| const [seriesName, seasonName] = fullText.split(' - ').map(part => part.trim()); | ||
| return { seriesName, seasonName }; | ||
| } | ||
|
|
||
| // If no series part found, return just the name | ||
| return { seriesName: fullText, seasonName: null }; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Fix name and nullability: getSeriesAndSeasonTitle.
- Typo in exported name.
- Return type must allow nulls when
is missing.
Apply:
-export const getSerieasAndSeasonTitle = (el: HTMLElement): { seriesName: string; seasonName: string | null } => {
+export const getSeriesAndSeasonTitle = (
+ el: HTMLElement
+): { seriesName: string | null; seasonName: string | null } => {
const titleElement = el.querySelector('h1');
if (!titleElement) {
- return { seriesName: null, seasonName: null };
+ return { seriesName: null, seasonName: null };
}
const fullText = titleElement.innerText.trim();
// Check if there's a series part indicated by ' - '
if (fullText.includes(' - ')) {
const [seriesName, seasonName] = fullText.split(' - ').map(part => part.trim());
return { seriesName, seasonName };
}
// If no series part found, return just the name
- return { seriesName: fullText, seasonName: null };
+ return { seriesName: fullText, seasonName: null };
};Remember to update all imports/usages (movie.service.ts).
🏁 Script executed:
#!/bin/bash
rg -n "getSerieasAndSeasonTitle|getSeriesAndSeasonTitle" --type=tsLength of output: 344
🏁 Script executed:
#!/bin/bash
# Inspect the import and usage context in movie.service.ts
sed -n '25,35p' src/services/movie.service.ts
echo "---"
sed -n '58,68p' src/services/movie.service.tsLength of output: 657
Fix name and nullability: getSeriesAndSeasonTitle.
- Typo in exported name:
getSerieasAndSeasonTitle→getSeriesAndSeasonTitle - Return type must allow nulls for
seriesName(code returns null when<h1>is missing, but type declaresstring)
Apply:
-export const getSerieasAndSeasonTitle = (el: HTMLElement): { seriesName: string; seasonName: string | null } => {
+export const getSeriesAndSeasonTitle = (
+ el: HTMLElement
+): { seriesName: string | null; seasonName: string | null } => {
const titleElement = el.querySelector('h1');
if (!titleElement) {
return { seriesName: null, seasonName: null };
}
const fullText = titleElement.innerText.trim();
// Check if there's a series part indicated by ' - '
if (fullText.includes(' - ')) {
const [seriesName, seasonName] = fullText.split(' - ').map(part => part.trim());
return { seriesName, seasonName };
}
// If no series part found, return just the name
return { seriesName: fullText, seasonName: null };
};Update import in src/services/movie.service.ts line 29.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getSerieasAndSeasonTitle = (el: HTMLElement): { seriesName: string; seasonName: string | null } => { | |
| const titleElement = el.querySelector('h1'); | |
| if (!titleElement) { | |
| return { seriesName: null, seasonName: null }; | |
| } | |
| const fullText = titleElement.innerText.trim(); | |
| // Check if there's a series part indicated by ' - ' | |
| if (fullText.includes(' - ')) { | |
| const [seriesName, seasonName] = fullText.split(' - ').map(part => part.trim()); | |
| return { seriesName, seasonName }; | |
| } | |
| // If no series part found, return just the name | |
| return { seriesName: fullText, seasonName: null }; | |
| }; | |
| export const getSeriesAndSeasonTitle = ( | |
| el: HTMLElement | |
| ): { seriesName: string | null; seasonName: string | null } => { | |
| const titleElement = el.querySelector('h1'); | |
| if (!titleElement) { | |
| return { seriesName: null, seasonName: null }; | |
| } | |
| const fullText = titleElement.innerText.trim(); | |
| // Check if there's a series part indicated by ' - ' | |
| if (fullText.includes(' - ')) { | |
| const [seriesName, seasonName] = fullText.split(' - ').map(part => part.trim()); | |
| return { seriesName, seasonName }; | |
| } | |
| // If no series part found, return just the name | |
| return { seriesName: fullText, seasonName: null }; | |
| }; |
🤖 Prompt for AI Agents
In src/helpers/movie.helper.ts around lines 23 to 39, rename the exported
function from getSerieasAndSeasonTitle to getSeriesAndSeasonTitle and change the
return type to { seriesName: string | null; seasonName: string | null } so the
function signature matches the code path that returns null when the <h1> is
missing; update any internal references accordingly. Then update the import in
src/services/movie.service.ts at line 29 to import getSeriesAndSeasonTitle
(instead of the misspelled name). Ensure the file exports the corrected
identifier and run a quick search to update any remaining call sites.
src/helpers/movie.helper.ts
Outdated
| export const getSeasonorEpisodeParent = (el: HTMLElement, serie?: { id: number; name: string; }): CSFDParent | null => { | ||
| const parents = el.querySelectorAll('.film-header h2 a'); | ||
| if (parents.length === 0) { | ||
| if (!serie) return null; | ||
| return { series: serie, season: null }; | ||
| } | ||
|
|
||
| const [parentSeries, parentSeason] = parents; | ||
|
|
||
| const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href')); | ||
| const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href')); | ||
| const seriesName = parentSeries?.textContent?.trim() || null; | ||
| const seasonName = parentSeason?.textContent?.trim() || null; | ||
|
|
||
| const series = seriesId && seriesName ? { id: seriesId, name: seriesName } : null; | ||
| const season = seasonId && seasonName ? { id: seasonId, name: seasonName } : null; | ||
|
|
||
| if (!series && !season) return null; | ||
|
|
||
| return { series, season }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid passing undefined to parseIdFromUrl and align with CSFDParent nullability.
- parentSeason may be missing; ensure string fallback for href.
- With CSFDParent updated to allow nulls, this function becomes type-safe.
Apply:
- const [parentSeries, parentSeason] = parents;
+ const [parentSeries, parentSeason] = parents;
- const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href'));
- const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href'));
+ const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href') || '');
+ const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href') || '');
const seriesName = parentSeries?.textContent?.trim() || null;
const seasonName = parentSeason?.textContent?.trim() || null;
const series = seriesId && seriesName ? { id: seriesId, name: seriesName } : null;
const season = seasonId && seasonName ? { id: seasonId, name: seasonName } : null;
if (!series && !season) return null;
return { series, season };Add tests for: 0 parents (uses provided serie), 1 parent (series only), 2 parents (series + season).
🏁 Script executed:
#!/bin/bash
rg -n "getSeasonorEpisodeParent" --type=ts -C2Length of output: 1191
Now let me search the codebase directly for the implementations needed to verify the review:
Let me search the codebase now:Now I need to check the codebase directly using shell scripts:
#!/bin/bash
# Find parseIdFromUrl function definition
echo "=== Searching for parseIdFromUrl definition ==="
rg -n "parseIdFromUrl" --type=ts -A 5 -B 2
echo ""
echo "=== Searching for CSFDParent type definition ==="
rg -n "CSFDParent" --type=ts -A 3 -B 2
echo ""
echo "=== Searching for test files related to getSeasonorEpisodeParent ==="
fd "\.test\.|\.spec\." --type=f | head -20Based on my comprehensive verification, I need to note that:
-
Optional chaining behavior: When a reference is null or undefined, the expression returns undefined instead of causing an error. The code uses
parentSeason?.getAttribute('href'), which will returnundefinedifparentSeasonis null/undefined. -
The concern in the review: The suggestion to add
|| ''assumes thatparseIdFromUrleither:- Doesn't accept
undefinedas a parameter, OR - Doesn't handle it correctly
- Doesn't accept
-
The code pattern: The current code destructures from
parentsarray which could have 0, 1, or 2 elements. When there's only 1 element,parentSeasonwill beundefined.
Without access to the actual codebase files through shell execution, and given the review comment's specific concerns about type safety and the request for tests, I'll provide a rewritten comment based on the general TypeScript best practices evident from the web search results:
Type-safe handling of optional element and improved test coverage needed.
The function currently relies on array destructuring that may assign undefined to parentSeason when fewer than 2 elements exist. This causes getAttribute('href') to return undefined, which is then passed to parseIdFromUrl.
Ensure parseIdFromUrl handles undefined input appropriately, or add explicit fallback:
parentSeason?.getAttribute('href') || ''provides a safer empty string fallback- This aligns with strict null checking requirements
Tests needed for edge cases:
- 0 parents (uses provided
serieparameter) - 1 parent (series only, season is undefined)
- 2 parents (series + season)
#!/bin/bash
echo "=== Searching for parseIdFromUrl definition ==="
rg -n "parseIdFromUrl\s*=" --type=ts -A 10 | head -30
echo ""
echo "=== Searching for CSFDParent type definition ==="
rg -n "type CSFDParent|interface CSFDParent" --type=ts -A 5
echo ""
echo "=== Searching for test files ==="
find . -name "*.test.ts" -o -name "*.spec.ts" | grep -i "movie\|helper"
echo ""
echo "=== Checking if CSFDParent has been updated ==="
rg -n "CSFDParent" --type=ts | grep -v "import\|export"🏁 Script executed:
#!/bin/bash
echo "=== Search for parseIdFromUrl implementation ==="
rg -n "const parseIdFromUrl|function parseIdFromUrl|export.*parseIdFromUrl" --type=ts -A 8
echo ""
echo "=== Search for CSFDParent type definition ==="
rg -n "type CSFDParent|interface CSFDParent" --type=ts -A 6
echo ""
echo "=== Search for test files containing getSeasonorEpisodeParent ==="
rg -n "getSeasonorEpisodeParent" --type=ts -l | grep -E "\.(test|spec)\.ts$"
echo ""
echo "=== Check all files importing or using parseIdFromUrl ==="
rg -n "parseIdFromUrl" --type=ts -B 3 -A 1Length of output: 8474
Add string fallback for optional chaining and expand test coverage.
The issue is valid: parentSeason?.getAttribute('href') returns undefined when parentSeason is null/undefined, and parseIdFromUrl expects a string type parameter (line 4, src/helpers/global.helper.ts). While the function handles falsy values gracefully, TypeScript will flag this as a type error in strict mode.
Apply the suggested fix:
- const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href'));
- const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href'));
+ const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href') || '');
+ const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href') || '');This ensures a string is always passed. Empty string will be falsy in parseIdFromUrl and safely return null.
Also add tests for edge cases: 0 parents (uses provided serie), 1 parent (series only), 2 parents (series + season). No existing tests found for getSeasonorEpisodeParent.
🤖 Prompt for AI Agents
In src/helpers/movie.helper.ts around lines 239 to 259, the calls
parentSeries?.getAttribute('href') and parentSeason?.getAttribute('href') can be
undefined and TypeScript strict mode flags that parseIdFromUrl expects a string;
change those calls to provide a string fallback (e.g.
parentSeries?.getAttribute('href') || '') so parseIdFromUrl always receives a
string and still returns null for falsy input, and add unit tests covering the
three edge cases: 0 parents (passes through provided serie), 1 parent (series
only), and 2 parents (series + season) to validate outputs.
| similar: getMovieBoxMovies(asideEl, 'Podobné'), | ||
| seasons: seasonOrEpisodeListType === 'seasons' ? getSeasonsOrEpisodes(el) : null, | ||
| episodes: seasonOrEpisodeListType === 'episodes' ? getSeasonsOrEpisodes(el) : null, | ||
| parent: (type === 'seriál') ? null : getSeasonorEpisodeParent(el, { id: movieId, name: title }), | ||
| episodeCode: type === 'epizoda' ? getEpisodeCode(el) : null, | ||
| seasonName, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Parent/seasons/episodes population: verify nullability contract and absolute URLs.
- After adjusting CSFDParent to allow nullable members, this assignment is fine.
- Ensure getSeasonsOrEpisodes returns items with non-null id/name/url and absolute URLs to avoid downstream consumers breaking.
To verify field presence:
🏁 Script executed:
#!/bin/bash
rg -nC2 "seasons:|episodes:|parent:|episodeCode:|seasonName" src --type=tsLength of output: 3403
---
🏁 Script executed:
```shell
#!/bin/bash
# Find CSFDParent type definition and getSeasonsOrEpisodes return type
rg -nC5 "type CSFDParent|interface CSFDParent|getSeasonsOrEpisodes.*return|export.*getSeasonsOrEpisodes" src --type=ts
Length of output: 1168
🏁 Script executed:
#!/bin/bash
# Find CSFDSeason type definition and related types
rg -nC3 "type CSFDSeason|interface CSFDSeason" src --type=tsLength of output: 279
🏁 Script executed:
#!/bin/bash
# Search for getSeasonorEpisodeParent implementation details
rg -nC8 "getSeasonorEpisodeParent\s*=" src --type=tsLength of output: 1162
🏁 Script executed:
#!/bin/bash
# Search for getSeasonsOrEpisodes implementation
rg -nC8 "getSeasonsOrEpisodes\s*=" src --type=tsLength of output: 1169
---
🏁 Script executed:
```shell
#!/bin/bash
# Get complete getSeasonorEpisodeParent implementation
rg -nA20 "export const getSeasonorEpisodeParent" src/helpers/movie.helper.ts
Length of output: 934
🏁 Script executed:
#!/bin/bash
# Get complete getSeasonsOrEpisodes implementation including URL handling
rg -nA25 "export const getSeasonsOrEpisodes" src/helpers/movie.helper.tsLength of output: 1166
🏁 Script executed:
#!/bin/bash
# Check for any recent changes to CSFDParent or related code
cat src/dto/movie.ts | head -40Length of output: 919
Critical type contract violations in parent/seasons/episodes population require fixes.
The original review comment identified valid concerns that remain unaddressed:
-
CSFDParent type contract violated: The
getSeasonorEpisodeParentfunction (line 243) returns{ series: serie, season: null }, and line 258 can return objects where bothseasonandseriesare null, but the CSFDParent interface requires both as non-nullable{ id: number; name: string; }objects. -
getSeasonsOrEpisodes returns nullable fields: The function (lines 213-214) assigns null to
nameandurlfields via the|| nullfallback, violating the CSFDSeason interface contract which requires both fields as non-nullable strings. -
URLs are relative, not absolute: Line 214 returns
getAttribute('href')which produces relative URLs (e.g.,/film/...), not absolute URLs (e.g.,https://www.csfd.cz/film/...), causing downstream consumers to fail.
Requires action:
- Make CSFDParent.season and CSFDParent.series optional/nullable in the type definition
- Ensure getSeasonsOrEpisodes provides non-null name/url or handle null cases
- Prepend
https://www.csfd.czto relative URLs in getSeasonsOrEpisodes and getSeasonorEpisodeParent
🤖 Prompt for AI Agents
In src/services/movie.service.ts around lines 99-105, the
parent/seasons/episodes population violates the TypeScript contracts and returns
relative/nullable fields; update the CSFDParent type so season and series are
optional or nullable, modify getSeasonsOrEpisodes to never return null for
name/url (e.g., coerce empty strings or filter out items with missing values)
and ensure you build absolute URLs by prepending "https://www.csfd.cz" to any
relative href returned by getAttribute('href'), and update
getSeasonorEpisodeParent to return values that match the updated CSFDParent
shape (use nullable/optional fields and construct absolute URLs for any hrefs it
returns).
da3c2fe to
2039636
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/helpers/movie.helper.ts (3)
23-39: Fix typo and nullability in getSeriesAndSeasonTitle; update imports/usages.Rename function and allow
seriesNameto be null when<h1>is missing.Apply:
-export const getSerieasAndSeasonTitle = (el: HTMLElement): { seriesName: string; seasonName: string | null } => { +export const getSeriesAndSeasonTitle = ( + el: HTMLElement +): { seriesName: string | null; seasonName: string | null } => { const titleElement = el.querySelector('h1'); if (!titleElement) { return { seriesName: null, seasonName: null }; } const fullText = titleElement.innerText.trim(); // Check if there's a series part indicated by ' - ' if (fullText.includes(' - ')) { const [seriesName, seasonName] = fullText.split(' - ').map(part => part.trim()); return { seriesName, seasonName }; } // If no series part found, return just the name return { seriesName: fullText, seasonName: null }; };Remember to update all imports/usages in
src/services/movie.service.ts.
200-218: Ensure non-null id/name/url and build absolute URLs in getSeasonsOrEpisodes.Current mapper can emit nulls and relative URLs; downstream consumers will break.
Apply:
-export const getSeasonsOrEpisodes = (el: HTMLElement, serie?: { id: number; title: string; }): CSFDSeason[] | null => { +export const getSeasonsOrEpisodes = ( + el: HTMLElement, + _serie?: { id: number; title: string } +): CSFDSeason[] | null => { const childrenList = el.querySelector('.film-episodes-list'); if (!childrenList) return null; - const childrenNodes = childrenList.querySelectorAll('.film-title'); - if (!childrenNodes?.length) return []; - - return childrenNodes.map((season) => { - const nameContainer = season.querySelector('.film-title-name'); - const infoContainer = season.querySelector('.info'); - - return { - id: parseLastIdFromUrl(nameContainer?.getAttribute('href') || ''), - name: nameContainer?.textContent?.trim() || null, - url: nameContainer?.getAttribute('href') || null, - info: infoContainer?.textContent?.replace(/[{()}]/g, '').trim() || null, - }; - }); + const childrenNodes = childrenList.querySelectorAll('.film-title'); + if (!childrenNodes?.length) return []; + + const items: CSFDSeason[] = []; + for (const node of childrenNodes) { + const nameEl = node.querySelector('.film-title-name'); + const infoEl = node.querySelector('.info'); + const href = nameEl?.getAttribute('href') || ''; + const id = parseLastIdFromUrl(href); + const name = nameEl?.textContent?.trim(); + if (!id || !name || !href) continue; + items.push({ + id, + name, + url: `https://www.csfd.cz${href}`, + info: infoEl?.textContent?.replace(/[{()}]/g, '').trim() || null, + }); + } + return items; }Note:
_serieis currently unused; either remove the param or pass it from the caller for future enrichment.
239-259: Do not pass undefined to parseIdFromUrl; confirm CSFDParent nullability. Also, consider renaming to getSeasonOrEpisodeParent.Provide string fallbacks for hrefs; result remains unchanged functionally. If
CSFDParentpermits nullable members, this is type-safe; otherwise, update the DTO.Apply:
const [parentSeries, parentSeason] = parents; - const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href')); - const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href')); + const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href') || ''); + const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href') || ''); const seriesName = parentSeries?.textContent?.trim() || null; const seasonName = parentSeason?.textContent?.trim() || null; const series = seriesId && seriesName ? { id: seriesId, name: seriesName } : null; const season = seasonId && seasonName ? { id: seasonId, name: seasonName } : null; if (!series && !season) return null; return { series, season };Optional: rename the function to
getSeasonOrEpisodeParentfor readability and update imports.src/services/movie.service.ts (1)
6-8: Rename import and usage to getSeriesAndSeasonTitle; avoid unsafe type assertion.Fix the helper name everywhere; consider avoiding
as CSFDFilmTypesifgetMovieTypemay return other values.Apply:
import { detectSeasonOrEpisodeListType, getEpisodeCode, @@ - getSeasonorEpisodeParent, - getSeasonsOrEpisodes, - getSerieasAndSeasonTitle + getSeasonorEpisodeParent, + getSeasonsOrEpisodes, + getSeriesAndSeasonTitle } from '../helpers/movie.helper'; @@ - const type = getMovieType(el) as CSFDFilmTypes; - const { seriesName = null, seasonName = null } = type === 'série' ? getSerieasAndSeasonTitle(el) : {}; + const type = getMovieType(el) as CSFDFilmTypes; + const { seriesName = null, seasonName = null } = type === 'série' ? getSeriesAndSeasonTitle(el) : {};Optional: instead of an assertion, narrow
typeby comparing the extracted text to your union of known values.Also applies to: 27-30, 61-66
🧹 Nitpick comments (3)
demo.ts (1)
5-5: Use an ID that showcases series/seasons to exercise new fields.Align the demo with the PR description (1431651) or another known series/season/episode ID so the new properties are visible when running the demo.
Apply:
-csfd.movie(621073).then((movie) => console.log(movie)); +csfd.movie(1431651).then((movie) => console.log(movie));src/helpers/movie.helper.ts (2)
220-229: Avoid capturing the year as episode code.The parentheses match can grab
(2018); exclude years.Apply:
- const match = text.match(/\(([^)]+)\)/); + const match = text.match(/\((?!(?:19|20)\d{2}\b)([^)]+)\)/);Add tests for typical formats:
S01E03,1/6,#1.1, and year-only titles.
231-237: Make list-type detection robust to case/spacing.Normalize text to lower-case; keep logic unchanged.
Apply:
- const headerText = el.querySelector('.box-header h3')?.innerText.trim() ?? ''; + const headerText = el.querySelector('.box-header h3')?.innerText?.trim().toLowerCase() ?? ''; - if (headerText.includes('Série')) return 'seasons'; - if (headerText.startsWith('Epizody')) return 'episodes'; + if (headerText.includes('série')) return 'seasons'; + if (headerText.startsWith('epizody')) return 'episodes';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
demo.ts(1 hunks)src/dto/movie.ts(3 hunks)src/helpers/global.helper.ts(1 hunks)src/helpers/movie.helper.ts(2 hunks)src/services/movie.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/dto/movie.ts
- src/helpers/global.helper.ts
🔇 Additional comments (3)
demo.ts (1)
11-11: Commenting out creator call is fine for a focused demo.Keeps output concise while testing series/seasons/episodes.
src/services/movie.service.ts (2)
102-105: Parent and seasonName wiring look good given helper fixes.Once the helper naming/URL/nullability fixes land, these fields should resolve correctly.
Ensure
CSFDParentallows nullableseries/seasonas returned bygetSeasonorEpisodeParent.
100-105: Episodes assignment likely mismatched with DTO type; pass series context and ensure mapper type aligns.
getSeasonsOrEpisodesreturnsCSFDSeason[], butepisodestypically expectsCSFDEpisode[]. Either make a dedicated episodes mapper or unify the item type. Also, pass the series context if you intend to use it.Apply:
- seasons: seasonOrEpisodeListType === 'seasons' ? getSeasonsOrEpisodes(el) : null, - episodes: seasonOrEpisodeListType === 'episodes' ? getSeasonsOrEpisodes(el) : null, + // provide context; adjust mapper/DTO if episodes use a distinct type + seasons: seasonOrEpisodeListType === 'seasons' ? getSeasonsOrEpisodes(el, { id: movieId, title }) : null, + episodes: seasonOrEpisodeListType === 'episodes' ? getSeasonsOrEpisodes(el, { id: movieId, title }) : null,Run to verify DTO expectations:
bartholomej
force-pushed
the
tv-series
branch
from
2039636 to
8ed4dae
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/dto/movie.ts (1)
27-30: CSFDParent fields must be nullable.getSeasonorEpisodeParent can produce missing season/series; the current non-null types are incorrect.
Apply:
-export interface CSFDParent { - season: { id: number; name: string; }; - series: { id: number; name: string; }; -} +/** + * Parent relationship for seasons/episodes. + * At least one of season/series is non-null at runtime. + */ +export interface CSFDParent { + season: { id: number; name: string } | null; + series: { id: number; name: string } | null; +}
🧹 Nitpick comments (2)
src/dto/movie.ts (1)
20-25: Episodes typed as CSFDSeason[]: define a dedicated CSFDEpisode and use it.Episodes are not seasons; align the public API with the PR description and future parsing needs.
Apply:
export interface CSFDMovie extends CSFDScreening { @@ - episodes: CSFDSeason[] | null; + episodes: CSFDEpisode[] | null; @@ } +/** + * Episode metadata parsed from CSFD. + */ +export interface CSFDEpisode { + id: number; + name: string; + url: string; // absolute + info: string | null; + episodeCode?: string | null; +}src/services/movie.service.ts (1)
103-104: episodeCode/seasonName population: OK, but ensure DTO alignment.These fields match the DTO. Once CSFDEpisode is introduced, consider moving episodeCode into the per‑episode shape to avoid top-level sparsity.
I can open a follow‑up PR to adjust types and helper returns once you confirm the desired shapes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
demo.ts(1 hunks)src/dto/movie.ts(3 hunks)src/helpers/global.helper.ts(1 hunks)src/helpers/movie.helper.ts(2 hunks)src/services/movie.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/helpers/global.helper.ts
- demo.ts
- src/helpers/movie.helper.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/movie.service.ts (2)
src/helpers/movie.helper.ts (8)
getMovieType(271-274)getSerieasAndSeasonTitle(23-39)detectSeasonOrEpisodeListType(231-237)getMovieTitle(41-43)getMovieYear(82-90)getSeasonsOrEpisodes(200-218)getSeasonorEpisodeParent(239-259)getEpisodeCode(220-229)src/dto/global.ts (1)
CSFDFilmTypes(25-36)
⏰ 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)
src/services/movie.service.ts (2)
65-69: Title resolution looks good.Preferring seriesName for 'série' pages is appropriate.
61-66: Fix helper function name typo; 'série' and 'seriál' are distinct types.
getSerieasAndSeasonTitleis misspelled (should begetSeriesAndSeasonTitle). However, 'série' and 'seriál' are separate valid types in CSFDFilmTypes and are intentionally used in different contexts—no normalization needed.
src/services/movie.service.ts
Outdated
| parent: (type === 'seriál') ? null : getSeasonorEpisodeParent(el, { id: movieId, name: title }), | ||
| episodeCode: type === 'epizoda' ? getEpisodeCode(el) : null, | ||
| seasonName, | ||
| }; |
There was a problem hiding this comment.
Parent computation bug: incorrect fallback sets a parent for films.
parent: (type === 'seriál') ? null : getSeasonorEpisodeParent(el, { id: movieId, name: title })
- For non‑series films, this passes the current movie as a “series” fallback, yielding a bogus parent.
- Compute parent only for seasons/episodes and avoid passing the fallback unless you truly know the series context.
Apply:
- parent: (type === 'seriál') ? null : getSeasonorEpisodeParent(el, { id: movieId, name: title }),
+ // Only derive parent on episode/season pages; no fallback to current movieId for films
+ parent:
+ (type === 'epizoda' || type === 'série')
+ ? getSeasonorEpisodeParent(el)
+ : null,Optionally, if you must supply a series fallback, pass it only when you have strong evidence of the series context (e.g., header breadcrumbs parsed).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/services/movie.service.ts around lines 102-105, the parent field
currently computes a fallback parent for non-series films causing incorrect
parent assignments; change the logic to compute parent only when the item is a
season or episode (i.e., when type indicates 'seriál' or 'epizoda' as
appropriate) and do not call getSeasonorEpisodeParent for films. If you must
provide a series fallback, only pass a fallback object when you have explicit
series context (e.g., header/breadcrumbs); otherwise set parent to null for
non-series entries.
8ed4dae to
80336e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/helpers/global.helper.ts (1)
14-22: Type signature and zero-coercion issues persist.The past review correctly identified these critical issues, but they remain unfixed in the current code:
- Type mismatch: Function declares return type
numberbut executesreturn null(lines 18, 20), which will fail understrictNullChecks.- Zero-coercion bug: Pattern
+id || null(line 18) incorrectly coerces the valid ID0tonull, losing data.Apply the previously suggested fix:
-export const parseLastIdFromUrl = (url: string): number => { +export const parseLastIdFromUrl = (url: string): number | null => { if (url) { - const idSlug = url?.split('/')[3]; - const id = idSlug?.split('-')[0]; - return +id || null; + const idSlug = url.split('/')[3]; + const num = Number((idSlug ?? '').split('-')[0]); + return Number.isFinite(num) ? num : null; } else { return null; } };Also apply the same pattern to
parseIdFromUrl(lines 4-12) for consistency.src/dto/movie.ts (2)
27-30: Make CSFDParent members nullable to match implementation.The implementation
getSeasonorEpisodeParent(lines 243, 253-254 inmovie.helper.ts) explicitly returnsnullfor eitherseasonorseries, but this interface declares both as non-null. This creates a type contract violation.Apply the previously suggested fix:
export interface CSFDParent { - season: { id: number; name: string; }; - series: { id: number; name: string; }; + season: { id: number; name: string } | null; + series: { id: number; name: string } | null; }
166-171: Fix type contract: CSFDSeason fields vs getSeasonsOrEpisodes implementation.The
CSFDSeasoninterface declaresnameandurlas non-null strings, butgetSeasonsOrEpisodes(lines 213-214 inmovie.helper.ts) explicitly returns them asstring | nullvia|| nullfallbacks. This type mismatch will cause issues.Either widen the DTO to match reality:
export interface CSFDSeason { id: number; - name: string; - url: string; + name: string | null; + url: string | null; info: string | null; }Or fix the helper to filter incomplete nodes and guarantee non-null values (preferred approach, as suggested in the past review).
src/helpers/movie.helper.ts (3)
23-39: Fix function name typo and return type mismatch.Two issues flagged in past review remain:
- Typo:
getSerieasAndSeasonTitleshould begetSeriesAndSeasonTitle(note the extra "a")- Type mismatch: Return type declares
seriesName: stringbut line 26 returns{ seriesName: null, seasonName: null }Apply the previously suggested fix:
-export const getSerieasAndSeasonTitle = (el: HTMLElement): { seriesName: string; seasonName: string | null } => { +export const getSeriesAndSeasonTitle = ( + el: HTMLElement +): { seriesName: string | null; seasonName: string | null } => { const titleElement = el.querySelector('h1'); if (!titleElement) { return { seriesName: null, seasonName: null }; } const fullText = titleElement.innerText.trim(); // Check if there's a series part indicated by ' - ' if (fullText.includes(' - ')) { const [seriesName, seasonName] = fullText.split(' - ').map(part => part.trim()); return { seriesName, seasonName }; } // If no series part found, return just the name return { seriesName: fullText, seasonName: null }; };Remember to update the import in
movie.service.tsline 29.
200-218: Filter invalid entries and build absolute URLs.The past review correctly identified that this function returns nullable
id/name/urlvalues and relative URLs, violating theCSFDSeasoncontract (which declares them non-null). This should be fixed to maintain type safety.Apply a filtering approach to ensure only valid entries are returned:
-export const getSeasonsOrEpisodes = (el: HTMLElement, serie?: { id: number; title: string; }): CSFDSeason[] | null => { +export const getSeasonsOrEpisodes = ( + el: HTMLElement, + _serie?: { id: number; title: string } +): CSFDSeason[] | null => { const childrenList = el.querySelector('.film-episodes-list'); if (!childrenList) return null; const childrenNodes = childrenList.querySelectorAll('.film-title'); if (!childrenNodes?.length) return []; - return childrenNodes.map((season) => { - const nameContainer = season.querySelector('.film-title-name'); - const infoContainer = season.querySelector('.info'); - - return { - id: parseLastIdFromUrl(nameContainer?.getAttribute('href') || ''), - name: nameContainer?.textContent?.trim() || null, - url: nameContainer?.getAttribute('href') || null, - info: infoContainer?.textContent?.replace(/[{()}]/g, '').trim() || null, - }; - }); + const items: CSFDSeason[] = []; + for (const node of childrenNodes) { + const nameEl = node.querySelector('.film-title-name'); + const infoEl = node.querySelector('.info'); + const href = nameEl?.getAttribute('href') || ''; + const id = parseLastIdFromUrl(href); + const name = nameEl?.textContent?.trim(); + if (!id || !name || !href) continue; + items.push({ + id, + name, + url: `https://www.csfd.cz${href}`, + info: infoEl?.textContent?.replace(/[{()}]/g, '').trim() || null, + }); + } + return items; }This ensures
CSFDSeasoncan maintain non-null contracts forid,name, andurl.
239-259: Add string fallback for optional chaining to prevent undefined.The past review correctly identified that
getAttribute('href')returnsundefinedwhen the element doesn't exist (lines 248-249), butparseIdFromUrlexpects astringparameter. TypeScript strict mode will flag this.Apply the previously suggested fix:
const [parentSeries, parentSeason] = parents; - const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href')); - const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href')); + const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href') || ''); + const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href') || ''); const seriesName = parentSeries?.textContent?.trim() || null; const seasonName = parentSeason?.textContent?.trim() || null; const series = seriesId && seriesName ? { id: seriesId, name: seriesName } : null; const season = seasonId && seasonName ? { id: seasonId, name: seasonName } : null; if (!series && !season) return null; return { series, season };This ensures
parseIdFromUrlalways receives a string, and the empty string will correctly returnnullper the function's logic.
🧹 Nitpick comments (1)
src/helpers/movie.helper.ts (1)
231-237: Consider using nullish coalescing for consistency.Minor suggestion: Use
??instead of logical OR for the default value to be more explicit about handling null/undefined.- const headerText = el.querySelector('.box-header h3')?.innerText.trim() ?? ''; + const headerText = el.querySelector('.box-header h3')?.innerText?.trim() ?? '';This ensures
trim()is only called ifinnerTextexists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
demo.ts(1 hunks)src/dto/movie.ts(3 hunks)src/helpers/global.helper.ts(1 hunks)src/helpers/movie.helper.ts(2 hunks)src/services/movie.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- demo.ts
- src/services/movie.service.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/helpers/movie.helper.ts (2)
src/helpers/global.helper.ts (2)
parseIdFromUrl(4-12)parseLastIdFromUrl(14-22)src/dto/movie.ts (2)
CSFDSeason(166-171)CSFDParent(27-30)
⏰ 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 (3)
src/dto/movie.ts (1)
20-25: LGTM: New fields appropriately nullable.The new fields on
CSFDMoviecorrectly declare nullable types (| null), which aligns with the conditional parsing logic inmovie.helper.ts.src/helpers/movie.helper.ts (2)
9-16: LGTM: Imports added correctly.New imports for
CSFDParent,CSFDSeason, andparseLastIdFromUrlare properly structured and necessary for the new functionality.
220-229: LGTM: Episode code extraction logic is sound.The function properly handles missing elements and uses regex to extract the episode code from parentheses. The nullable return type correctly reflects the optional nature of this field.
bartholomej
force-pushed
the
tv-series
branch
2 times, most recently
from
288840c to
c788bb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/helpers/global.helper.ts (1)
14-22: Type mismatch and 0-coercion bug remain unfixed.The past review flagged these critical issues, but the code has not been updated:
Type signature mismatch: Function declares
numberreturn type but executesreturn null(lines 18, 20). This will fail understrictNullChecks.0-coercion bug: Pattern
+id || null(line 18) incorrectly converts the valid ID0tonull. UseNumber.isFinite()instead.Apply the fix from the previous review:
-export const parseLastIdFromUrl = (url: string): number => { +export const parseLastIdFromUrl = (url: string): number | null => { if (url) { - const idSlug = url?.split('/')[3]; - const id = idSlug?.split('-')[0]; - return +id || null; + const idSlug = url.split('/')[3]; + const num = Number((idSlug ?? '').split('-')[0]); + return Number.isFinite(num) ? num : null; } else { return null; } };Also apply the same fix to
parseIdFromUrl(lines 4-12) for consistency.src/dto/movie.ts (2)
27-30: CSFDParent type contract violation remains unfixed.Past review correctly identified that
getSeasonorEpisodeParent(in movie.helper.ts:243, 258) returns objects whereseasonorseriescan benull, but this interface declares both as non-nullable.Apply the suggested fix:
export interface CSFDParent { - season: { id: number; name: string; }; - series: { id: number; name: string; }; + season: { id: number; name: string } | null; + series: { id: number; name: string } | null; }
166-171: CSFDSeason type contract mismatch remains unfixed.Past review correctly identified that
getSeasonsOrEpisodes(movie.helper.ts:213-214) returnsnameandurlasstring | nullvia|| nullfallbacks, but this interface declares both as non-nullable strings.Either widen the DTO to accept nulls:
export interface CSFDSeason { id: number; - name: string; - url: string; + name: string | null; + url: string | null; info: string | null; }Or fix
getSeasonsOrEpisodesto filter out incomplete nodes and build absolute URLs (preferred approach detailed in the helper file review).src/helpers/movie.helper.ts (3)
23-39: Function name typo and type mismatch remain unfixed.Past review flagged two issues that persist:
- Typo:
getSerieasAndSeasonTitleshould begetSeriesAndSeasonTitle- Type mismatch: Return type declares
seriesName: stringbut line 26 returnsnullApply the fix:
-export const getSerieasAndSeasonTitle = (el: HTMLElement): { seriesName: string; seasonName: string | null } => { +export const getSeriesAndSeasonTitle = ( + el: HTMLElement +): { seriesName: string | null; seasonName: string | null } => { const titleElement = el.querySelector('h1'); if (!titleElement) { return { seriesName: null, seasonName: null }; } const fullText = titleElement.innerText.trim(); // Check if there's a series part indicated by ' - ' if (fullText.includes(' - ')) { const [seriesName, seasonName] = fullText.split(' - ').map(part => part.trim()); return { seriesName, seasonName }; } // If no series part found, return just the name return { seriesName: fullText, seasonName: null }; };Update the import in
src/services/movie.service.tsline 29.
200-218: Type contract violations and relative URLs remain unfixed.Past review flagged critical issues that persist in the current code:
- Lines 213-214 return nullable values (
|| null) violating CSFDSeason contract (requires non-nullnameandurl)- Line 214 returns relative URLs without base domain (e.g.,
/film/...instead ofhttps://www.csfd.cz/film/...)Apply the suggested fix:
-export const getSeasonsOrEpisodes = (el: HTMLElement, serie?: { id: number; title: string; }): CSFDSeason[] | null => { +export const getSeasonsOrEpisodes = ( + el: HTMLElement, + _serie?: { id: number; title: string } +): CSFDSeason[] | null => { const childrenList = el.querySelector('.film-episodes-list'); if (!childrenList) return null; const childrenNodes = childrenList.querySelectorAll('.film-title'); if (!childrenNodes?.length) return []; - return childrenNodes.map((season) => { - const nameContainer = season.querySelector('.film-title-name'); - const infoContainer = season.querySelector('.info'); - - return { - id: parseLastIdFromUrl(nameContainer?.getAttribute('href') || ''), - name: nameContainer?.textContent?.trim() || null, - url: nameContainer?.getAttribute('href') || null, - info: infoContainer?.textContent?.replace(/[{()}]/g, '').trim() || null, - }; - }); + const items: CSFDSeason[] = []; + for (const node of childrenNodes) { + const nameEl = node.querySelector('.film-title-name'); + const infoEl = node.querySelector('.info'); + const href = nameEl?.getAttribute('href') || ''; + const id = parseLastIdFromUrl(href); + const name = nameEl?.textContent?.trim(); + if (!id || !name || !href) continue; + items.push({ + id, + name, + url: `https://www.csfd.cz${href}`, + info: infoEl?.textContent?.replace(/[{()}]/g, '').trim() || null, + }); + } + return items; }
239-259: Type safety issue with undefined passing remains unfixed.Past review correctly identified that
parentSeason?.getAttribute('href')returnsundefinedwhenparentSeasonis null/undefined (e.g., whenparentsarray has only 1 element), butparseIdFromUrlexpects astringparameter.Apply the suggested fix:
const [parentSeries, parentSeason] = parents; - const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href')); - const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href')); + const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href') || ''); + const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href') || ''); const seriesName = parentSeries?.textContent?.trim() || null; const seasonName = parentSeason?.textContent?.trim() || null; const series = seriesId && seriesName ? { id: seriesId, name: seriesName } : null; const season = seasonId && seasonName ? { id: seasonId, name: seasonName } : null; if (!series && !season) return null; return { series, season };Also add test coverage for: 0 parents (uses fallback
serie), 1 parent (series only), 2 parents (series + season).src/services/movie.service.ts (1)
100-105: Multiple critical issues in parent/seasons/episodes population.Three problems persist:
Lines 100-101:
getSeasonsOrEpisodesreturns nullable fields and relative URLs, violating CSFDSeason contract (see movie.helper.ts:200-218).Line 102 parent computation bug: Logic
(type === 'seriál') ? null : getSeasonorEpisodeParent(...)incorrectly assigns a parent to regular films. The intent appears inverted—only episodes/seasons should have parents, not top-level series.Line 102 fallback misuse: Passing
{ id: movieId, name: title }as the series fallback for non-series items creates bogus self-references.Fix the parent computation logic:
- parent: (type === 'seriál') ? null : getSeasonorEpisodeParent(el, { id: movieId, name: title }), + // Only episodes/seasons have parents; films/series do not + parent: + (type === 'epizoda' || type === 'série') + ? getSeasonorEpisodeParent(el) + : null,And ensure
getSeasonsOrEpisodesis fixed per the movie.helper.ts review.
🧹 Nitpick comments (2)
src/dto/movie.ts (1)
20-25: New fields align with TV series support.The added fields (
seasons,episodes,parent,episodeCode,seasonName) appropriately extend the movie model for series/season/episode data. Note:episodescurrently reusesCSFDSeason[]type; consider a dedicatedCSFDEpisodetype in the future for clarity.src/helpers/movie.helper.ts (1)
231-237: Detection logic is straightforward.The string matching approach works for the current CSFD markup. Consider adding test coverage for the Czech string patterns to catch future markup changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
demo.ts(1 hunks)src/dto/movie.ts(3 hunks)src/helpers/global.helper.ts(1 hunks)src/helpers/movie.helper.ts(2 hunks)src/services/movie.service.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/services/movie.service.ts (2)
src/helpers/movie.helper.ts (8)
getMovieType(271-274)getSerieasAndSeasonTitle(23-39)detectSeasonOrEpisodeListType(231-237)getMovieTitle(41-43)getMovieYear(82-90)getSeasonsOrEpisodes(200-218)getSeasonorEpisodeParent(239-259)getEpisodeCode(220-229)src/dto/global.ts (1)
CSFDFilmTypes(25-36)
demo.ts (2)
src/index.ts (2)
csfd(66-72)movie(39-42)src/services/movie.service.ts (1)
movie(36-52)
src/helpers/movie.helper.ts (2)
src/helpers/global.helper.ts (2)
parseIdFromUrl(4-12)parseLastIdFromUrl(14-22)src/dto/movie.ts (2)
CSFDSeason(166-171)CSFDParent(27-30)
🔇 Additional comments (3)
demo.ts (1)
7-7: LGTM! Demo updates align with new TV series functionality.The movie ID change and commented creator call are appropriate for testing the new season/episode features introduced in this PR.
Also applies to: 13-13
src/helpers/movie.helper.ts (1)
220-229: LGTM! Episode code extraction is correctly implemented.The regex pattern safely extracts content from parentheses, with proper null handling for missing elements.
src/services/movie.service.ts (1)
61-65: Title computation logic depends on unfixed helper issues.Lines 62 and 65 rely on
getSerieasAndSeasonTitle, which has:
- A typo in the function name
- Type mismatch (returns null but declares string for
seriesName)Once the helper is fixed (see review comment on movie.helper.ts:23-39), update the import on line 29 to use the corrected function name.
bartholomej
force-pushed
the
tv-series
branch
from
c788bb9 to
fcce368
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/dto/movie.ts (1)
20-24: Fix CSFDParent / CSFDSeason type contracts to match helper behavior.
CSFDParentandCSFDSeasoncurrently declare fields as non‑nullable, but the helpers return nulls:
getSeasonorEpisodeParentcan produce{ series: null, season: {…} },{ series: {…}, season: null }, ornull, soseasonandseriesmust allownull.getSeasonsOrEpisodescan emit entries wherenameandurlarenull, andidcan benullfromparseLastIdFromUrl.This violates the published DTO contract and will hurt TS consumers (especially with
strictNullChecks).Recommend:
- Make
CSFDParentmembers nullable:-export interface CSFDParent { - season: { id: number; name: string; }; - series: { id: number; name: string; }; -} +export interface CSFDParent { + season: { id: number; name: string } | null; + series: { id: number; name: string } | null; +}
- Either:
- Keep
CSFDSeasonstrict and fixgetSeasonsOrEpisodesto filter out incomplete nodes and always return non‑nullid/name/url(preferred), or- Widen DTO fields to nullable (
id: number | null; name: string | null; url: string | null;) if you truly want to propagate partial items.Right now the implementation and typings are inconsistent.
Also applies to: 27-30
src/helpers/movie.helper.ts (3)
23-39: Rename and fix nullability ingetSerieasAndSeasonTitle.Issues:
- Exported name has a typo (
getSerieasAndSeasonTitle).- Return type says
seriesName: string, but you returnseriesName: nullwhen<h1>is missing.Recommend:
-export const getSerieasAndSeasonTitle = (el: HTMLElement): { seriesName: string; seasonName: string | null } => { +export const getSeriesAndSeasonTitle = ( + el: HTMLElement +): { seriesName: string | null; seasonName: string | null } => { const titleElement = el.querySelector('h1'); if (!titleElement) { return { seriesName: null, seasonName: null }; } const fullText = titleElement.innerText.trim(); if (fullText.includes(' - ')) { const [seriesName, seasonName] = fullText.split(' - ').map((part) => part.trim()); return { seriesName, seasonName }; } return { seriesName: fullText, seasonName: null }; };Then update the import and call site in
src/services/movie.service.tsto usegetSeriesAndSeasonTitle.
200-218: HardengetSeasonsOrEpisodes: avoid nulls, fix.trim()chain, and normalize URLs.Current implementation has several problems:
name,url, andidcan benull, butCSFDSeasondeclares them as non‑null.infoContainer?.textContent?.replace(/…/g, '').trim()will throw ifinfoContaineris missing, because.trim()is not behind optional chaining.urlis returned as a relative path; elsewhere you expose absolute URLs.Suggested rewrite:
-export const getSeasonsOrEpisodes = (el: HTMLElement, serie?: { id: number; title: string; }): CSFDSeason[] | null => { +export const getSeasonsOrEpisodes = (el: HTMLElement): CSFDSeason[] | null => { const childrenList = el.querySelector('.film-episodes-list'); if (!childrenList) return null; const childrenNodes = childrenList.querySelectorAll('.film-title'); if (!childrenNodes?.length) return []; - return childrenNodes.map((season) => { - const nameContainer = season.querySelector('.film-title-name'); - const infoContainer = season.querySelector('.info'); - - return { - id: parseLastIdFromUrl(nameContainer?.getAttribute('href') || ''), - name: nameContainer?.textContent?.trim() || null, - url: nameContainer?.getAttribute('href') || null, - info: infoContainer?.textContent?.replace(/[{()}]/g, '').trim() || null, - }; - }); + const items: CSFDSeason[] = []; + for (const node of childrenNodes) { + const nameEl = node.querySelector('.film-title-name'); + const infoEl = node.querySelector('.info'); + const href = nameEl?.getAttribute('href') || ''; + const id = parseLastIdFromUrl(href); + const name = nameEl?.textContent?.trim(); + + if (!id || !name || !href) continue; + + const rawInfo = infoEl?.textContent + ?.replace(/[{()}]/g, '') + .trim(); + + items.push({ + id, + name, + url: href.startsWith('http') ? href : `https://www.csfd.cz${href}`, + info: rawInfo || null, + }); + } + return items; }This keeps
CSFDSeasonnon‑nullable, avoids a potential runtime error on.trim(), and makes URLs consistent with the rest of the helpers.
239-258: AligngetSeasonorEpisodeParentwith types and avoid passing undefined toparseIdFromUrl.Issues:
parseIdFromUrl(parentSeries?.getAttribute('href'))can receiveundefined; the helper expects a string.- Function can return
{ series: null, season: {...} }or{ series: {...}, season: null }, butCSFDParentcurrently requires both fields non‑null.- With
parents.length === 0, you fabricate a parent{ series: serie, season: null }which is then used even for plain films via the service fallback, yielding a bogus “self‑parent”.At minimum:
- const [parentSeries, parentSeason] = parents; - - const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href')); - const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href')); + const [parentSeries, parentSeason] = parents; + + const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href') || ''); + const seasonId = parseIdFromUrl(parentSeason?.getAttribute('href') || ''); const seriesName = parentSeries?.textContent?.trim() || null; const seasonName = parentSeason?.textContent?.trim() || null; const series = seriesId && seriesName ? { id: seriesId, name: seriesName } : null; const season = seasonId && seasonName ? { id: seasonId, name: seasonName } : null; if (!series && !season) return null; return { series, season };Combined with updating
CSFDParentto allow nullable members, this becomes type‑safe and matches real DOM possibilities. Also consider dropping theseriefallback entirely and just returningnullwhenparents.length === 0; the movie service can then decide when to compute a parent.src/services/movie.service.ts (2)
61-66: Fix helper name usage and clarify title/seasonName derivation.Once
getSerieasAndSeasonTitleis renamed and its types fixed, update this call accordingly:- const type = getMovieType(el) as CSFDFilmTypes; - const { seriesName = null, seasonName = null } = type === 'série' ? getSerieasAndSeasonTitle(el) : {}; + const type = getMovieType(el) as CSFDFilmTypes; + const { seriesName = null, seasonName = null } = + type === 'série' ? getSeriesAndSeasonTitle(el) : {};The
titleassignment logic (type === 'série' && seriesName ? seriesName : getMovieTitle(el)) looks reasonable and keeps the DTO’s title focused on the series name for season pages.Also applies to: 99-105
100-104: Correctparentcomputation: avoid bogus self‑parent for films and restrict to seasons/episodes.Current logic:
parent: (type === 'seriál') ? null : getSeasonorEpisodeParent(el, { id: movieId, name: title }),For plain films (non‑series), this calls
getSeasonorEpisodeParentwith a fallback{ id: movieId, name: title }, and the helper returns{ series: serie, season: null }when no parents are found. That incorrectly assigns every film a “parent” equal to itself.Recommend:
- parent: (type === 'seriál') ? null : getSeasonorEpisodeParent(el, { id: movieId, name: title }), + // Only derive parent for season/episode detail pages; films and root series have no parent. + parent: + type === 'epizoda' || type === 'série' + ? getSeasonorEpisodeParent(el) + : null,This aligns
parentwith the real hierarchy (episodes/seasons have parents; films and main series entries do not).
🧹 Nitpick comments (3)
src/dto/movie.ts (1)
20-22: Consider a distinct episode DTO instead of reusing CSFDSeason.
episodes: CSFDSeason[] | nullreuses the season type for episodes. If episode items differ even slightly (e.g., include an episode code, air date, or per‑episode info vs per‑season info), a dedicatedCSFDEpisodeinterface will keep the model clearer and safer to evolve.If the structures are truly identical, adding a short docstring clarifying that
CSFDSeasonis also used for episode list entries would help future readers.Also applies to: 166-171
src/helpers/movie.helper.ts (1)
231-237: MakedetectSeasonOrEpisodeListTypemore robust.Right now it inspects only the first
.box-header h3. If the seasons/episodes section isn’t the first box, you’ll silently returnnull.Consider scanning all headers instead:
-export const detectSeasonOrEpisodeListType = (el: HTMLElement) => { - const headerText = el.querySelector('.box-header h3')?.innerText.trim() ?? ''; - - if (headerText.includes('Série')) return 'seasons'; - if (headerText.startsWith('Epizody')) return 'episodes'; - return null; -} +export const detectSeasonOrEpisodeListType = (el: HTMLElement): 'seasons' | 'episodes' | null => { + const headers = el.querySelectorAll('.box-header h3'); + for (const header of headers) { + const text = header.innerText.trim(); + if (text.includes('Série')) return 'seasons'; + if (text.startsWith('Epizody')) return 'episodes'; + } + return null; +}Not critical, but makes the detection less brittle if CSFD changes box order.
src/services/movie.service.ts (1)
100-103: Ensure seasons/episodes arrays and DTOs stay consistent with helper behavior.The wiring:
seasons: seasonOrEpisodeListType === 'seasons' ? getSeasonsOrEpisodes(el) : null, episodes: seasonOrEpisodeListType === 'episodes' ? getSeasonsOrEpisodes(el) : null, episodeCode: type === 'epizoda' ? getEpisodeCode(el) : null,is fine conceptually, but depends on
getSeasonsOrEpisodesreturning items that matchCSFDSeason(non‑null id/name/url, absolute URLs). Once you adjust the helper as suggested, this assignment will be consistent and safe for consumers. Until then, the DTO over‑promises non‑nullable fields.After updating the helper, consider adding a couple of integration tests hitting a known series, season, and episode page to lock in expected shapes of
seasons,episodes,parent, andepisodeCode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
demo.ts(1 hunks)src/dto/movie.ts(3 hunks)src/helpers/global.helper.ts(1 hunks)src/helpers/movie.helper.ts(2 hunks)src/services/movie.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/helpers/global.helper.ts
🧰 Additional context used
🧬 Code graph analysis (3)
demo.ts (2)
src/index.ts (2)
csfd(70-76)movie(39-42)src/services/movie.service.ts (1)
movie(36-52)
src/helpers/movie.helper.ts (2)
src/helpers/global.helper.ts (2)
parseIdFromUrl(4-12)parseLastIdFromUrl(14-22)src/dto/movie.ts (2)
CSFDSeason(166-171)CSFDParent(27-30)
src/services/movie.service.ts (1)
src/helpers/movie.helper.ts (6)
getSerieasAndSeasonTitle(23-39)detectSeasonOrEpisodeListType(231-237)getMovieTitle(41-43)getSeasonsOrEpisodes(200-218)getSeasonorEpisodeParent(239-259)getEpisodeCode(220-229)
🔇 Additional comments (1)
demo.ts (1)
7-7: Demo change looks fine.Switching the demo movie ID and commenting out the creator call is harmless and keeps the example simple.
bartholomej
force-pushed
the
tv-series
branch
from
fcce368 to
5169dba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/mocks/series2-episode.mock.ts (1)
1-1234: Same fixture pattern and trade-offs as other large mocksThis follows the same “full-page HTML in a template string” approach as the other mocks. Functionally fine, but the same comments about noisy diffs and readability apply; see note on
series1-season1-episode.mock.tsfor possible ways to externalize or slim these fixtures.src/helpers/movie.helper.ts (3)
109-127: Fix function name typo and return type mismatch.This issue was flagged in a previous review but remains unresolved:
- Typo:
getSerieasAndSeasonTitleshould begetSeriesAndSeasonTitle- Type mismatch: Return type declares
seriesName: stringbut Line 114 returnsnull, causing a type error in strict mode.Apply the previously suggested fix:
-export const getSerieasAndSeasonTitle = ( +export const getSeriesAndSeasonTitle = ( el: HTMLElement -): { seriesName: string; seasonName: string | null } => { +): { seriesName: string | null; seasonName: string | null } => { const titleElement = el.querySelector('h1'); if (!titleElement) { return { seriesName: null, seasonName: null }; } const fullText = titleElement.innerText.trim(); // Check if there's a series part indicated by ' - ' if (fullText.includes(' - ')) { const [seriesName, seasonName] = fullText.split(' - ').map((part) => part.trim()); return { seriesName, seasonName }; } // If no series part found, return just the name return { seriesName: fullText, seasonName: null }; };Update the import in
src/services/movie.service.ts(line 29 per previous review).
288-309: Ensure non-nullable fields and absolute URLs in getSeasonsOrEpisodes.A previous review flagged this but the issue persists:
- Lines 304-305:
nameandurlcan benull, butCSFDSeasonlikely requires non-null values- Line 305: URLs are relative paths, not absolute
- Line 303: If
parseLastIdFromUrlreturnsnullfor invalid input,idbecomesnull(coerced from the return value)Filter out invalid entries and construct absolute URLs:
export const getSeasonsOrEpisodes = ( el: HTMLElement, - serie?: { id: number; title: string } + _serie?: { id: number; title: string } ): CSFDSeason[] | null => { const childrenList = el.querySelector('.film-episodes-list'); if (!childrenList) return null; const childrenNodes = childrenList.querySelectorAll('.film-title'); if (!childrenNodes?.length) return []; - return childrenNodes.map((season) => { - const nameContainer = season.querySelector('.film-title-name'); - const infoContainer = season.querySelector('.info'); - - return { - id: parseLastIdFromUrl(nameContainer?.getAttribute('href') || ''), - name: nameContainer?.textContent?.trim() || null, - url: nameContainer?.getAttribute('href') || null, - info: infoContainer?.textContent?.replace(/[{()}]/g, '').trim() || null - }; - }); + const items: CSFDSeason[] = []; + for (const node of childrenNodes) { + const nameEl = node.querySelector('.film-title-name'); + const infoEl = node.querySelector('.info'); + const href = nameEl?.getAttribute('href') || ''; + const id = parseLastIdFromUrl(href); + const name = nameEl?.textContent?.trim(); + if (!id || !name || !href) continue; + items.push({ + id, + name, + url: `https://www.csfd.cz${href}`, + info: infoEl?.textContent?.replace(/[{()}]/g, '').trim() || null + }); + } + return items; }
330-358: Add string fallback to prevent undefined being passed to parseIdFromUrl.This issue was flagged in a previous review and remains unresolved:
Line 347 passes
parentSeries?.getAttribute('href')(which can beundefined) toparseIdFromUrl, but the function signature insrc/helpers/global.helper.ts(lines 3-11) expects astringparameter.Apply the fix from the previous review:
- const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href')); - const seasonId = parseLastIdFromUrl(parentSeason?.getAttribute('href') || ''); + const seriesId = parseIdFromUrl(parentSeries?.getAttribute('href') || ''); + const seasonId = parseLastIdFromUrl(parentSeason?.getAttribute('href') || '');This ensures both calls always pass a string. Empty strings will be handled by the
if (url)check inparseIdFromUrl, returningnullsafely.Also consider adding unit tests for edge cases (0, 1, 2 parents) as suggested in the previous review.
🧹 Nitpick comments (7)
tests/mocks/series1-season1-episode.mock.ts (1)
1-1277: Huge inline HTML fixture: consider externalizing for maintainabilityThis mock is valid and structurally rich, but inlining the full CSFD page as a template string makes diffs noisy and the test file hard to navigate. Consider moving the raw HTML into a
.htmlfixture file and loading it viafs.readFileSyncin tests, or at least documenting which parts are relied on by the parser to reduce future accidental edits.tests/mocks/series1-season1.mock.ts (1)
1-1357: Season mock is structurally correct; same comments on fixture sizeThis season mock appears structurally aligned with how the parser operates (episodes list, creators, JSON-LD, aside). As with the other mocks, consider external fixture files or minimal, focused HTML samples in future to reduce inline bulk, but there’s no functional issue here.
tests/movie.test.ts (2)
636-654: getEpisodeCode tests are well‑structured and cover main branchesThe three tests here validate:
- Extraction of a code in parentheses,
- Absence of code in the title,
- Missing
.film-header-name h1entirely.This matches the helper’s behavior (regex on the
<h1>text) and gives good coverage of success and null cases. No changes needed.You could also add one test that uses a full episode mock DOM (like from the HTML fixtures) to guard against future selector changes, but that’s optional given the integration tests in
tests/series.test.ts.
656-669: getSeasonorEpisodeParent tests only cover the fallback pathThese tests exercise the fallback behavior (no header anchors, with/without
serieargument), which is good. Note that the primary parsing path—where anchors exist under.film-header h2 aor.film-header h1 aand IDs are parsed viaparseIdFromUrl/parseLastIdFromUrl—is only exercised indirectly intests/series.test.ts.Given that, there’s no immediate change needed here, but if
getSeasonorEpisodeParentevolves further, consider adding a small unit test with synthetic header links to lock in the selector/ID parsing behavior.tests/series.test.ts (2)
1-28: parseMovieMock: solid test helper, but relies on a private methodUsing
parseMovieMockto drive realistic HTML throughMovieScraper.buildMovieis a good approach to avoid network I/O and directly validate parsing. The only trade‑off is calling the privatebuildMovievia(scraper as any), which couples tests to internal structure.If
buildMovie’s signature changes often, consider:
- Adding a thin public test‑only wrapper in
MovieScraper(e.g.fromHtml(...)), or- Exporting
buildMoviebehind a non‑public symbol guarded by/** @internal */.Not urgent, but it would make these tests less brittle to refactors in
movie.service.ts.
394-440: Cross‑pattern validations are sensible sanity checksThe shared validation block nicely consolidates:
- Basic invariants (positive IDs and
/film/URLs).- Mutual exclusivity for seasons vs episodes on main vs season pages.
- Episode parent references and presence of
episodeCode.- Behavior for series without seasons (episodes directly,
seasons = null).This is a good high‑level safety net over the more detailed tests above. If test runtime ever becomes an issue, you might cache parsed
CSFDMovieinstances instead of reparsing the same mocks in each test, but that’s an optimization, not a requirement.src/helpers/movie.helper.ts (1)
322-328: Consider i18n support for header text detection.The function hard-codes Czech text patterns ('Série', 'Epizody'). If the codebase supports multiple languages (based on
getLocalizedCreatorLabel), this may need localization.Example enhancement:
export const detectSeasonOrEpisodeListType = ( el: HTMLElement, language?: string ) => { const headerText = el.querySelector('.box-header h3')?.innerText.trim() ?? ''; const patterns = { cs: { seasons: 'Série', episodes: 'Epizody' }, sk: { seasons: 'Série', episodes: 'Epizódy' }, en: { seasons: 'Season', episodes: 'Episode' } }; const lang = patterns[language || 'cs'] || patterns.cs; if (headerText.includes(lang.seasons)) return 'seasons'; if (headerText.startsWith(lang.episodes)) return 'episodes'; return null; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/helpers/movie.helper.ts(5 hunks)tests/fetchers.test.ts(1 hunks)tests/mocks/series1-season1-episode.mock.ts(1 hunks)tests/mocks/series1-season1.mock.ts(1 hunks)tests/mocks/series2-episode.mock.ts(1 hunks)tests/movie.test.ts(6 hunks)tests/series.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/series.test.ts (2)
src/dto/movie.ts (1)
CSFDMovie(3-25)src/services/movie.service.ts (1)
MovieScraper(35-107)
tests/fetchers.test.ts (3)
src/services/movie.service.ts (1)
movie(36-51)src/dto/movie.ts (1)
CSFDMovie(3-25)src/dto/global.ts (1)
CSFDFilmTypes(25-36)
tests/movie.test.ts (1)
src/helpers/movie.helper.ts (3)
getSerieasAndSeasonTitle(109-127)getEpisodeCode(311-320)getSeasonorEpisodeParent(330-358)
src/helpers/movie.helper.ts (2)
src/dto/movie.ts (6)
CSFDCreatorGroups(145-158)CSFDCreatorGroupsEnglish(161-174)CSFDCreatorGroupsSlovak(176-186)CSFDParent(27-30)CSFDMovieCreator(80-91)CSFDBoxContent(195-195)src/helpers/global.helper.ts (2)
parseLastIdFromUrl(14-22)parseIdFromUrl(4-12)
🔇 Additional comments (7)
tests/movie.test.ts (2)
190-209: Updated trivia expectations look consistent with richer site contentThe adjusted expected trivia strings for movies and series (lines 194–197 and 205–209) match the style of CSFD “Zajímavosti” entries and keep the parser’s behavior constrained to the textual content. No issues here.
265-275: VOD expectation tweaks align with URL tracking paramsThe updated expected VOD URLs (e.g., added
utm_content=csfdand trailing URL fixes) are fine and keep tests strict about the scraper returning fully qualified links. Nothing else to change in this segment.tests/series.test.ts (3)
38-223: Pattern 1 (Simpsons): good coverage of series → season → episode behaviorThese three describe blocks thoroughly validate the “Series with Seasons” pattern:
- Main series page: seasons present, episodes absent, rating, creators, genres, no parent/episodeCode/seasonName.
- Season page: episodes present, seasons absent,
seasonNameset, parent containing only series.- Episode page: correct
title,type = 'epizoda',episodeCode, parent with both series and season, year, duration, creators, and descriptions.The assertions align with the provided mocks and the intended semantics of
CSFDMovie. No changes needed here.
225-312: Pattern 2 main series (The Curse): validates direct episodes correctlyThis suite checks the “Series with direct episodes” case as intended:
type = 'seriál', correct title/year, episodes list present, no seasons.- No parent/episodeCode/seasonName on the main series object.
- Ratings, creators, genres, VOD, and descriptions all asserted.
The additional check that
firstEpisode.infomatches/E\d+/is a nice way to ensure the episode code is captured into the list metadata. Looks good.
320-389: Pattern 2 episode (The Curse E01): parent and duration checks look correctThese tests correctly assert:
- Episode metadata (
title,type = 'epizoda',episodeCode = 'E01',year = 2023,duration = 61).- Parent has only a series (no season) with expected id/name.
- No seasons/episodes arrays on the episode object itself.
- Creators, genres, descriptions, and VOD entries exist.
This matches the provided mock HTML where there is only a series header link and no season context. No issues here.
tests/fetchers.test.ts (1)
155-252: LGTM! Comprehensive test coverage for series patterns.The test suite effectively covers both series patterns (with and without seasons) across all hierarchy levels: series, season, and episode pages. The assertions validate type, title, hierarchical relationships (parent series/season), and collection fields (seasons/episodes).
src/helpers/movie.helper.ts (1)
311-320: LGTM! Episode code extraction is robust.The function correctly handles missing elements and uses a simple regex pattern to extract the code from parentheses.
| describe('Get Series and Season Title', () => { | ||
| test('With series and season', () => { | ||
| const html = parse('<h1>Series Name - Season 1</h1>'); | ||
| const result = getSerieasAndSeasonTitle(html as any); | ||
| expect(result).toEqual({ seriesName: 'Series Name', seasonName: 'Season 1' }); | ||
| }); | ||
|
|
||
| test('Only series name', () => { | ||
| const html = parse('<h1>Movie Name</h1>'); | ||
| const result = getSerieasAndSeasonTitle(html as any); | ||
| expect(result).toEqual({ seriesName: 'Movie Name', seasonName: null }); | ||
| }); | ||
|
|
||
| test('No title element', () => { | ||
| const html = parse('<div></div>'); | ||
| const result = getSerieasAndSeasonTitle(html as any); | ||
| expect(result).toEqual({ seriesName: null, seasonName: null }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
getSerieasAndSeasonTitle: tests reveal a type mismatch in the helper
These tests correctly cover:
- Series + season header (
"Series Name - Season 1"), - Plain title without season,
- Missing
<h1>element.
However, getSerieasAndSeasonTitle is currently declared to return { seriesName: string; seasonName: string | null }, yet its implementation (and this “No title element” test) return seriesName: null when the <h1> is absent. This is a TypeScript type violation.
Consider changing the helper signature to:
export const getSerieasAndSeasonTitle = (
el: HTMLElement
): { seriesName: string | null; seasonName: string | null } => { ... }so the implementation and tests are type-safe and consistent.
🤖 Prompt for AI Agents
In tests/movie.test.ts around lines 616-634, the helper getSerieasAndSeasonTitle
is returning seriesName|null in some cases but its TypeScript signature
currently requires seriesName: string; change the helper's exported signature to
return { seriesName: string | null; seasonName: string | null } so the
implementation and tests are type-safe; update the function declaration/export
to use the nullable seriesName type and fix any call sites or imports
accordingly to satisfy the new return type.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/helpers/movie.helper.ts`:
- Around line 119-123: The current logic uses fullText.split(' - ') and
destructures into const [seriesName, seasonName], which drops any subsequent
segments (e.g., "Simpsonovi - Série 1 - Special"); change to split only on the
first occurrence by finding the first indexOf(' - ') and slicing: compute const
idx = fullText.indexOf(' - '), then set seriesName = fullText.slice(0,
idx).trim() and seasonName = fullText.slice(idx + 3).trim(); update the block
where fullText is handled and keep the same returned object shape ({ seriesName,
seasonName }) so additional ' - ' parts are preserved in seasonName.
- Around line 322-328: detectSeasonOrEpisodeListType currently matches
Czech-only header strings ('Série' / 'Epizody') so it will fail for 'en'/'sk';
change it to be language-aware by adding a language parameter (e.g.,
detectSeasonOrEpisodeListType(el: HTMLElement, language: string)) and use a
small localization map or reuse the project's localization helper pattern to map
each language to the expected "seasons" and "episodes" header texts, then test
headerText against the localized variants; update all call sites to pass
options.language so seasons/episodes are detected for cs|en|sk.
In `@tests/fetchers.test.ts`:
- Around line 173-176: The test "Seasons" (and the similar "episodes" assertion)
uses expect(movie.seasons).toBeDefined() which does not guard against null
before using the non-null assertion movie.seasons!.length; change the assertions
to first check not.toBeNull() (e.g., expect(movie.seasons).toBeDefined();
expect(movie.seasons).not.toBeNull();) before accessing movie.seasons!.length,
and do the same pattern for movie.episodes to avoid a TypeError and match the
pattern used in series.test.ts.
🧹 Nitpick comments (1)
tests/series.test.ts (1)
394-439: Cross-pattern tests re-parse mock HTML unnecessarily.Each test in this
describeblock callsparseMovieMockagain for the same mocks and IDs already parsed in the earlierdescribeblocks. Consider using a shared fixture (e.g., a module-scopedbeforeAllor a shared object) to avoid redundant parsing.
| // Check if there's a series part indicated by ' - ' | ||
| if (fullText.includes(' - ')) { | ||
| const [seriesName, seasonName] = fullText.split(' - ').map((part) => part.trim()); | ||
| return { seriesName, seasonName }; | ||
| } |
There was a problem hiding this comment.
split(' - ') discards segments when the title contains multiple - separators.
If a title like "Simpsonovi - Série 1 - Special" is encountered, the destructuring const [seriesName, seasonName] = ... silently drops the third part. Consider splitting only on the first occurrence:
Proposed fix
- if (fullText.includes(' - ')) {
- const [seriesName, seasonName] = fullText.split(' - ').map((part) => part.trim());
- return { seriesName, seasonName };
- }
+ const separatorIndex = fullText.indexOf(' - ');
+ if (separatorIndex !== -1) {
+ const seriesName = fullText.substring(0, separatorIndex).trim();
+ const seasonName = fullText.substring(separatorIndex + 3).trim();
+ return { seriesName, seasonName };
+ }📝 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.
| // Check if there's a series part indicated by ' - ' | |
| if (fullText.includes(' - ')) { | |
| const [seriesName, seasonName] = fullText.split(' - ').map((part) => part.trim()); | |
| return { seriesName, seasonName }; | |
| } | |
| // Check if there's a series part indicated by ' - ' | |
| const separatorIndex = fullText.indexOf(' - '); | |
| if (separatorIndex !== -1) { | |
| const seriesName = fullText.substring(0, separatorIndex).trim(); | |
| const seasonName = fullText.substring(separatorIndex + 3).trim(); | |
| return { seriesName, seasonName }; | |
| } |
🤖 Prompt for AI Agents
In `@src/helpers/movie.helper.ts` around lines 119 - 123, The current logic uses
fullText.split(' - ') and destructures into const [seriesName, seasonName],
which drops any subsequent segments (e.g., "Simpsonovi - Série 1 - Special");
change to split only on the first occurrence by finding the first indexOf(' - ')
and slicing: compute const idx = fullText.indexOf(' - '), then set seriesName =
fullText.slice(0, idx).trim() and seasonName = fullText.slice(idx + 3).trim();
update the block where fullText is handled and keep the same returned object
shape ({ seriesName, seasonName }) so additional ' - ' parts are preserved in
seasonName.
| export const detectSeasonOrEpisodeListType = (el: HTMLElement) => { | ||
| const headerText = el.querySelector('.box-header h3')?.innerText.trim() ?? ''; | ||
|
|
||
| if (headerText.includes('Série')) return 'seasons'; | ||
| if (headerText.startsWith('Epizody')) return 'episodes'; | ||
| return null; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the API supports language options that would change page content
rg -n "language" src/vars.ts -A 3
echo "---"
rg -n "language" src/services/movie.service.ts -B 1 -A 3Repository: bartholomej/node-csfd-api
Length of output: 2958
🏁 Script executed:
#!/bin/bash
# Find CSFDLanguage type definition
rg -n "CSFDLanguage" src/ -B 2 -A 5 | head -100
echo "---"
# Search for localization or translation constants
rg -n "Série|Epizody" src/ -B 2 -A 2
echo "---"
# Find where detectSeasonOrEpisodeListType is called
rg -n "detectSeasonOrEpisodeListType" src/ -B 2 -A 2Repository: bartholomej/node-csfd-api
Length of output: 4053
🏁 Script executed:
#!/bin/bash
# Check the getMovie function context
rg -n "export class MovieScraper\|getMovie\|detectSeasonOrEpisodeListType" src/services/movie.service.ts -B 2 -A 10 | head -150
echo "---"
# Check localization helpers
rg -n "getLocalizedCreatorLabel" src/helpers/ -B 2 -A 10 | head -80Repository: bartholomej/node-csfd-api
Length of output: 754
🏁 Script executed:
#!/bin/bash
# Check the getMovie function signature and full context
sed -n '30,80p' src/services/movie.service.tsRepository: bartholomej/node-csfd-api
Length of output: 2053
detectSeasonOrEpisodeListType is hardcoded to Czech header text.
The strings 'Série' and 'Epizody' are Czech-only. The API supports three languages ('cs', 'en', 'sk') and receives a language parameter in the options, but detectSeasonOrEpisodeListType does not receive or use it. When called with language: 'en' or language: 'sk', the page content would be in that language, causing detection to fail and silently return null, omitting seasons and episodes data.
Either add language-aware detection (following the pattern of getLocalizedCreatorLabel which already handles localization) or document this as a Czech-only limitation.
🤖 Prompt for AI Agents
In `@src/helpers/movie.helper.ts` around lines 322 - 328,
detectSeasonOrEpisodeListType currently matches Czech-only header strings
('Série' / 'Epizody') so it will fail for 'en'/'sk'; change it to be
language-aware by adding a language parameter (e.g.,
detectSeasonOrEpisodeListType(el: HTMLElement, language: string)) and use a
small localization map or reuse the project's localization helper pattern to map
each language to the expected "seasons" and "episodes" header texts, then test
headerText against the localized variants; update all call sites to pass
options.language so seasons/episodes are detected for cs|en|sk.
| test('Seasons', () => { | ||
| expect(movie.seasons).toBeDefined(); | ||
| expect(movie.seasons!.length).toBeGreaterThan(20); | ||
| }); |
There was a problem hiding this comment.
toBeDefined() does not guard against null — prefer not.toBeNull() before non-null assertion.
expect(movie.seasons).toBeDefined() passes when seasons is null, so movie.seasons!.length on Line 175 would throw a TypeError rather than a clean assertion failure. The same pattern appears on Line 199 for episodes. The mock-based tests in series.test.ts (Lines 59–61) correctly use both toBeDefined() and not.toBeNull() — align this file.
Proposed fix
test('Seasons', () => {
expect(movie.seasons).toBeDefined();
+ expect(movie.seasons).not.toBeNull();
expect(movie.seasons!.length).toBeGreaterThan(20);
});Same for Line 198:
test('Episodes', () => {
expect(movie.episodes).toBeDefined();
+ expect(movie.episodes).not.toBeNull();
expect(movie.episodes!.length).toBeGreaterThan(0);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('Seasons', () => { | |
| expect(movie.seasons).toBeDefined(); | |
| expect(movie.seasons!.length).toBeGreaterThan(20); | |
| }); | |
| test('Seasons', () => { | |
| expect(movie.seasons).toBeDefined(); | |
| expect(movie.seasons).not.toBeNull(); | |
| expect(movie.seasons!.length).toBeGreaterThan(20); | |
| }); |
🤖 Prompt for AI Agents
In `@tests/fetchers.test.ts` around lines 173 - 176, The test "Seasons" (and the
similar "episodes" assertion) uses expect(movie.seasons).toBeDefined() which
does not guard against null before using the non-null assertion
movie.seasons!.length; change the assertions to first check not.toBeNull()
(e.g., expect(movie.seasons).toBeDefined();
expect(movie.seasons).not.toBeNull();) before accessing movie.seasons!.length,
and do the same pattern for movie.episodes to avoid a TypeError and match the
pattern used in series.test.ts.
583f11e to
3087bc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/series.test.ts`:
- Around line 136-144: In the "Should have parent series info" test (and the
other tests referenced around lines 183–190 and 344–346), add explicit null
guards before accessing movie.parent!.series properties: insert
expect(movie.parent!.series).not.toBeNull() (or equivalent
expect(...).toBeDefined()) immediately before the existing assertions that read
movie.parent!.series.id and movie.parent!.series.name so the test fails with a
clear assertion instead of a TypeError when CSFDParent.series can be null.
🧹 Nitpick comments (3)
src/dto/movie.ts (1)
20-21:episodesreusesCSFDSeason[]type — consider a dedicatedCSFDEpisodetype or alias.The PR description lists
CSFDEpisode[]as the intended type forepisodes, but the implementation usesCSFDSeason[]. While the shape is currently identical, a type alias (type CSFDEpisode = CSFDSeason) would improve readability and leave room for future divergence.src/services/movie.service.ts (1)
60-65:buildMoviereturn type annotation is missing.The method previously had an explicit return type (likely
CSFDMovie) but it's been dropped. Adding the return type ensures the object literal is checked against the interface at compile time, which would catch thename: null/url: nullmismatch inCSFDSeason.Proposed fix
- ) { + ): CSFDMovie {tests/series.test.ts (1)
25-27: Accessing privatebuildMovievia reflection couples tests to implementation details.This is a common pragmatic pattern, but if
buildMovieis renamed or refactored, these tests break silently. Consider either makingbuildMoviea package-internal utility or extracting the parsing logic into a testable public function.
tests/series.test.ts
Outdated
| test('Should have parent series info', () => { | ||
| expect(movie.parent).toBeDefined(); | ||
| expect(movie.parent).not.toBeNull(); | ||
| expect(movie.parent!.series).toBeDefined(); | ||
| expect(movie.parent!.series.id).toBe(72489); | ||
| expect(movie.parent!.series.name).toBe('Simpsonovi'); | ||
| // Season page doesn't have a parent season, only parent series | ||
| expect(movie.parent!.season).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Add explicit null guards before accessing parent.series sub-properties.
Lines 140–141 access movie.parent!.series.id and .name directly. If series is null (per CSFDParent), this throws a TypeError instead of a clean assertion failure. Add expect(movie.parent!.series).not.toBeNull() before the property assertions. Same applies at lines 183–190 and 344–346.
Proposed fix (line 136-144)
test('Should have parent series info', () => {
expect(movie.parent).toBeDefined();
expect(movie.parent).not.toBeNull();
expect(movie.parent!.series).toBeDefined();
+ expect(movie.parent!.series).not.toBeNull();
expect(movie.parent!.series.id).toBe(72489);
expect(movie.parent!.series.name).toBe('Simpsonovi');
// Season page doesn't have a parent season, only parent series
expect(movie.parent!.season).toBeNull();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('Should have parent series info', () => { | |
| expect(movie.parent).toBeDefined(); | |
| expect(movie.parent).not.toBeNull(); | |
| expect(movie.parent!.series).toBeDefined(); | |
| expect(movie.parent!.series.id).toBe(72489); | |
| expect(movie.parent!.series.name).toBe('Simpsonovi'); | |
| // Season page doesn't have a parent season, only parent series | |
| expect(movie.parent!.season).toBeNull(); | |
| }); | |
| test('Should have parent series info', () => { | |
| expect(movie.parent).toBeDefined(); | |
| expect(movie.parent).not.toBeNull(); | |
| expect(movie.parent!.series).toBeDefined(); | |
| expect(movie.parent!.series).not.toBeNull(); | |
| expect(movie.parent!.series.id).toBe(72489); | |
| expect(movie.parent!.series.name).toBe('Simpsonovi'); | |
| // Season page doesn't have a parent season, only parent series | |
| expect(movie.parent!.season).toBeNull(); | |
| }); |
🤖 Prompt for AI Agents
In `@tests/series.test.ts` around lines 136 - 144, In the "Should have parent
series info" test (and the other tests referenced around lines 183–190 and
344–346), add explicit null guards before accessing movie.parent!.series
properties: insert expect(movie.parent!.series).not.toBeNull() (or equivalent
expect(...).toBeDefined()) immediately before the existing assertions that read
movie.parent!.series.id and movie.parent!.series.name so the test fails with a
clear assertion instead of a TypeError when CSFDParent.series can be null.
dde5398 to
c538e2f
Compare
- Rename CSFDSeason interface to CSFDSeriesChild to avoid ambiguity (the type is used for both season and episode list items) - Rename `name` field to `title` in CSFDSeriesChild and CSFDParent for consistency with the top-level CSFDMovie.title field - Update all usages in helpers, service and tests
2 participants
What's new and why I made this pull request?
Resolves #43
I've prepared the first version of the parsing for series, seasons, and episodes. Ready for testing – feel free to join in!
Just use
await csfd.movie(1431651)method and there are some brand new properties:Pull request type
Summary by CodeRabbit
New Features
Updates
Tests
Chore