Refactor CSFD API helpers and improve error handling #71
Conversation
- Fix `parseISO8601Duration` to correctly calculate total duration in minutes. - Refactor `fetchPage` to throw `CsfdError` instead of returning 'Error' string, and add retry logic with exponential backoff. - Refactor `src/helpers/global.helper.ts` and `src/helpers/movie.helper.ts` for better robustness and type safety. - Update tests to reflect changes and increase timeout for live tests. - Export `CsfdError` and `CSFDOptions` in `src/index.ts`. Co-authored-by: bartholomej <[email protected]>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThe PR enhances error handling in the fetch layer with a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant fetchPage
participant fetchSafe
participant Sleep
Caller->>fetchPage: fetchPage(url, options)
loop retry (max 3 attempts)
fetchPage->>fetchSafe: Merge headers & fetch
alt 4xx Status
fetchSafe-->>fetchPage: 4xx Error
fetchPage-->>Caller: throw CsfdError (no retry)
else 5xx Status
fetchSafe-->>fetchPage: 5xx Error
fetchPage->>Sleep: Wait exponential backoff
Sleep-->>fetchPage: Resume
else Success
fetchSafe-->>fetchPage: Response text
fetchPage-->>Caller: Return response
else Other Error
fetchSafe-->>fetchPage: Non-CsfdError
alt Retries exhausted
fetchPage-->>Caller: throw last error
else Retries remaining
fetchPage->>Sleep: Wait exponential backoff
Sleep-->>fetchPage: Resume
end
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #71 +/- ##
==========================================
- Coverage 99.52% 98.84% -0.68%
==========================================
Files 28 28
Lines 629 607 -22
Branches 143 133 -10
==========================================
- Hits 626 600 -26
- Misses 3 5 +2
- Partials 0 2 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/helpers/global.helper.ts (1)
58-59:⚠️ Potential issue | 🔴 CriticalBug:
sleepresolves immediately —msis never passed tosetTimeout.
setTimeout(res)uses the default delay of 0. Themsparameter is silently ignored.🐛 Fix
-export const sleep = (ms: number) => new Promise((res) => setTimeout(res)); +export const sleep = (ms: number) => new Promise((res) => setTimeout(res, ms));
🤖 Fix all issues with AI agents
In `@src/fetchers/index.ts`:
- Line 21: There are two implementations of sleep: the correct one in the sleep
constant in src/fetchers/index.ts and a broken duplicate in
src/helpers/global.helper.ts that calls setTimeout without the ms argument;
remove the duplicate and centralize the utility by keeping a single export
(e.g., export const sleep = (ms: number) => new Promise(resolve =>
setTimeout(resolve, ms)) in one file) then update callers to import that sleep
function (replace local definitions/usages in fetchers and global.helper with an
import), or alternatively fix the broken implementation in global.helper.ts to
accept and pass the ms parameter and export it for reuse so all modules import
the same symbol.
In `@src/helpers/movie.helper.ts`:
- Around line 77-80: getMovieId currently declares a return type of number but
calls parseIdFromUrl which returns number | null; update the function to either
return number | null (change signature to getMovieId(...): number | null and
propagate null when el.querySelector('.tabs .tab-nav-list a') is missing) or
keep a number return by providing a clear fallback (e.g., coalesce
parseIdFromUrl(...) ?? 0) and ensure the signature and any callers are adjusted
accordingly; reference getMovieId and parseIdFromUrl when making the change.
🧹 Nitpick comments (6)
src/fetchers/index.ts (2)
29-33: Arrow inforEachimplicitly returns a value frommergedHeaders.set().Biome flags this because
Headers.prototype.set()returns theHeadersobject and the arrow function implicitly returns it. Use a block body orvoidto silence the lint.♻️ Proposed fix
- reqHeaders.forEach((value, key) => mergedHeaders.set(key, value)); + reqHeaders.forEach((value, key) => { mergedHeaders.set(key, value); });
10-12: User-Agent is selected once at module load and reused for every request.
defaultHeadersis evaluated once when the module is first imported, so everyfetchPagecall in the same process uses the same random UA string. If per-request rotation is intended (e.g., to reduce scraping fingerprinting), move the random selection insidefetchPage.tests/fetchers.test.ts (1)
259-278: Try/catch error tests silently pass iffetchPagenever throws.If the URL happens to return a non-error status, no assertion runs and the test passes vacuously. Use
rejectsor guard withexpect.assertions(1).♻️ Proposed fix (using rejects)
describe('User page 404', () => { test('Fetch error URL', async () => { - try { - const url = userRatingsUrl(badId); - await fetchPage(url); - } catch (e) { - expect(e).toBeInstanceOf(CsfdError); - } + const url = userRatingsUrl(badId); + await expect(fetchPage(url)).rejects.toThrow(CsfdError); }); }); describe('Movie page 404', () => { test('Fetch error URL', async () => { - try { - const url = movieUrl(badId, {}); - await fetchPage(url); - } catch (e) { - expect(e).toBeInstanceOf(CsfdError); - } + const url = movieUrl(badId, {}); + await expect(fetchPage(url)).rejects.toThrow(CsfdError); }); });src/helpers/global.helper.ts (1)
4-12:parseIdFromUrlgrabs the first digit run in the entire URL — fragile if the domain/path ever contains leading digits.
/(\d+)/onhttps://csfd123.cz/film/906693-…would return123, not906693. Consider anchoring the match to the path segment (e.g.,/(\d+)-/or splitting on/first). For the currentcsfd.czdomain this works, but it's a latent risk.src/helpers/movie.helper.ts (2)
116-123:+jsonLd.dateCreated || nulltreats year0as null.Unlikely for movie years, but
+value || nullconflates0andNaNwith absence. Not a practical concern for this domain — just noting for awareness.
196-201:filter(Boolean)may not narrow the type tostring[]in all TS configurations.If the compiler complains, use an explicit predicate:
.filter((x): x is string => Boolean(x)). Otherwise, this is fine.
| } | ||
| } | ||
|
|
||
| const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); |
There was a problem hiding this comment.
Duplicate sleep — and the copy in global.helper.ts is broken.
A sleep function also exists in src/helpers/global.helper.ts (line 58), but that version omits the ms argument to setTimeout, making it resolve immediately regardless of the delay value. Consider removing the duplicate in global.helper.ts or fixing it and importing from a single location to avoid divergence.
#!/bin/bash
# Verify the buggy sleep in global.helper.ts
rg -n 'setTimeout' --type=ts -C2🤖 Prompt for AI Agents
In `@src/fetchers/index.ts` at line 21, There are two implementations of sleep:
the correct one in the sleep constant in src/fetchers/index.ts and a broken
duplicate in src/helpers/global.helper.ts that calls setTimeout without the ms
argument; remove the duplicate and centralize the utility by keeping a single
export (e.g., export const sleep = (ms: number) => new Promise(resolve =>
setTimeout(resolve, ms)) in one file) then update callers to import that sleep
function (replace local definitions/usages in fetchers and global.helper with an
import), or alternatively fix the broken implementation in global.helper.ts to
accept and pass the ms parameter and export it for reuse so all modules import
the same symbol.
| export const getMovieId = (el: HTMLElement): number => { | ||
| const url = el.querySelector('.tabs .tab-nav-list a').attributes.href; | ||
| const url = el.querySelector('.tabs .tab-nav-list a')?.attributes.href; | ||
| return parseIdFromUrl(url); | ||
| }; |
There was a problem hiding this comment.
Return type mismatch: getMovieId is typed number but parseIdFromUrl returns number | null.
If the anchor element is missing, parseIdFromUrl(undefined) returns null, which contradicts the declared : number return type. Either widen the return type to number | null or provide a fallback (e.g., ?? 0).
🐛 Option A — widen return type
-export const getMovieId = (el: HTMLElement): number => {
+export const getMovieId = (el: HTMLElement): number | null => {🐛 Option B — fallback to 0
- return parseIdFromUrl(url);
+ return parseIdFromUrl(url) ?? 0;#!/bin/bash
# Check if strict null checks are enabled in tsconfig
fd tsconfig.json --exec cat {}🤖 Prompt for AI Agents
In `@src/helpers/movie.helper.ts` around lines 77 - 80, getMovieId currently
declares a return type of number but calls parseIdFromUrl which returns number |
null; update the function to either return number | null (change signature to
getMovieId(...): number | null and propagate null when el.querySelector('.tabs
.tab-nav-list a') is missing) or keep a number return by providing a clear
fallback (e.g., coalesce parseIdFromUrl(...) ?? 0) and ensure the signature and
any callers are adjusted accordingly; reference getMovieId and parseIdFromUrl
when making the change.
2 participants
Implemented requested improvements including fixing duration parsing, adding robust error handling with retries in fetch logic, and refactoring helper functions for better maintainability and type safety. Verified with comprehensive test suite.
PR created automatically by Jules for task 17251613356509418357 started by @bartholomej
Summary by CodeRabbit
Bug Fixes
Chores