feat: integrate valibot for user ratings and reviews validation#73
feat: integrate valibot for user ratings and reviews validation#73bartholomej wants to merge 2 commits intomasterfrom
Conversation
- Added `valibot` dependency. - Implemented `CSFDUserRatingsSchema` and `CSFDUserReviewsSchema` for runtime validation. - Refactored `UserRatingsScraper` and `UserReviewsScraper` to use `safeParse`. - Fixed a bug in `getUserReviewType` extraction. - Invalid data is now safely skipped with a warning logged to the console. - Build size increased by ~6KB; performance impact is negligible. Co-authored-by: bartholomej <5861310+bartholomej@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughAdds valibot as a runtime dependency, introduces Valibot DTO schemas for user ratings and reviews, makes poster nullable, updates helper parsing, and changes services to validate and conditionally push items using safeParse/flatten with validation failure logging. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as Service Layer
participant Helper as Helper/Parser
participant Schema as Valibot Schema
participant Logger as Logger
Client->>Service: request user ratings/reviews
Service->>Helper: parse DOM / extract fields
Helper-->>Service: parsed fields (type, rating, etc.)
Service->>Service: build item object
Service->>Schema: safeParse(item)
alt Validation Success
Schema-->>Service: valid
Service-->>Client: include item in results
else Validation Failure
Schema-->>Service: issues
Service->>Logger: log warning (title, id, flattened issues)
Service-->>Service: skip item
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #73 +/- ##
==========================================
- Coverage 99.52% 98.16% -1.36%
==========================================
Files 28 30 +2
Lines 629 655 +26
Branches 143 148 +5
==========================================
+ Hits 626 643 +17
- Misses 3 12 +9 ☔ 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
🤖 Fix all issues with AI agents
In `@src/services/user-ratings.service.ts`:
- Around line 95-105: The call to buildUserRatings can throw before safeParse
runs, which can abort processing; wrap the buildUserRatings(el) invocation in a
try/catch inside the shouldProcess block (around where result =
safeParse(CSFDUserRatingsSchema, item) is called) so DOM-extraction errors are
caught, log a concise warning including el/id/title context, and continue to the
next item; only call safeParse when the build succeeded and push result.output
into films on success.
In `@src/services/user-reviews.service.ts`:
- Around line 97-107: buildUserReviews may throw (e.g., null DOM lookups) before
safeParse runs, causing the whole loop to abort; wrap the build+validation logic
for each element in a try/catch around the block that calls buildUserReviews(el)
and safeParse(CSFDUserReviewsSchema, item), log the error including identifying
fields if available, and skip pushing to films on catch or validation failure so
a single malformed review doesn't stop processing; reference buildUserReviews,
safeParse, CSFDUserReviewsSchema and films to locate the change.
🧹 Nitpick comments (3)
src/dto/user-ratings.schema.ts (1)
1-42: Consider usingv.picklist()to reduce boilerplate and keep schemas in sync with types.
v.picklist(['film', 'TV film', 'pořad', ...])is equivalent to a union of literals but more concise. Also, the literal values here duplicate theCSFDFilmTypes,CSFDColorRating, andCSFDStarstypes insrc/dto/global.ts. If a new film type is added to one but not the other, validation will silently reject valid data.Consider deriving the TypeScript types from the schemas using
v.InferOutput<typeof CSFDFilmTypesSchema>to maintain a single source of truth — or at least add a comment cross-referencing the types.src/services/user-reviews.service.ts (1)
80-81:getUserReviewTypeis called twice per element — once for filtering (line 81) and again insidebuildUserReviews(line 118).Consider passing the already-extracted
typeintobuildUserReviewsto avoid redundant DOM traversal.Also applies to: 113-125
src/services/user-ratings.service.ts (1)
78-79:getUserRatingTypecalled twice per element — same redundancy as in the reviews service.Consider passing the pre-extracted
typeintobuildUserRatings.Also applies to: 111-122
- Added `valibot` dependency. - Implemented `CSFDUserRatingsSchema` and `CSFDUserReviewsSchema` for runtime validation. - Refactored `UserRatingsScraper` and `UserReviewsScraper` to use `safeParse` and wrap extraction in `try-catch` for DOM safety. - Fixed a bug in `getUserReviewType` extraction. - Invalid or error-prone items are now safely skipped with a warning logged to the console. - Build size increased by ~6KB; performance impact is negligible. Co-authored-by: bartholomej <5861310+bartholomej@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/services/user-ratings.service.ts`:
- Around line 78-112: getUserRatingType(el) is called outside the try/catch and
can throw, aborting the loop; move the extraction into the try block so DOM
errors are caught, capture the returned type and pass it into buildUserRatings
to avoid re-extraction, and update buildUserRatings' signature to accept the
precomputed type (then use that value instead of calling getUserRatingType
again); keep the existing validation flow with safeParse(CSFDUserRatingsSchema)
and the existing warning logs.
| for (const el of movies) { | ||
| const type = getUserRatingType(el); | ||
|
|
||
| let shouldProcess = true; | ||
|
|
||
| // Filtering includesOnly | ||
| if (config?.includesOnly?.length) { | ||
| if (config.includesOnly.some((include) => type === include)) { | ||
| films.push(this.buildUserRatings(el)); | ||
| if (!config.includesOnly.some((include) => type === include)) { | ||
| shouldProcess = false; | ||
| } | ||
| // Filter excludes | ||
| } else if (config?.excludes?.length) { | ||
| if (!config.excludes.some((exclude) => type === exclude)) { | ||
| films.push(this.buildUserRatings(el)); | ||
| if (config.excludes.some((exclude) => type === exclude)) { | ||
| shouldProcess = false; | ||
| } | ||
| } | ||
|
|
||
| if (shouldProcess) { | ||
| try { | ||
| const item = this.buildUserRatings(el); | ||
| const result = safeParse(CSFDUserRatingsSchema, item); | ||
| if (result.success) { | ||
| films.push(result.output as CSFDUserRatings); | ||
| } else { | ||
| console.warn( | ||
| `Skipping invalid user rating. Title: ${item.title}, ID: ${item.id}`, | ||
| JSON.stringify(flatten(result.issues)) | ||
| ); | ||
| } | ||
| } catch (e) { | ||
| console.warn( | ||
| `Skipping user rating due to scraping error (DOM change?): ${(e as Error).message}` | ||
| ); | ||
| } | ||
| } else { | ||
| // Without filtering | ||
| films.push(this.buildUserRatings(el)); | ||
| } |
There was a problem hiding this comment.
getUserRatingType(el) on line 79 is outside the try/catch, so a DOM extraction error there will still abort the entire loop.
The try/catch on lines 96–111 protects buildUserRatings, but getUserRatingType(el) at line 79 performs the same kind of DOM scraping and can throw identically. An error there skips the catch block and propagates up, aborting all remaining items on the page.
Move the getUserRatingType call inside the try block (or wrap the entire loop body), and pass type into buildUserRatings to avoid calling it twice.
Proposed fix
for (const el of movies) {
- const type = getUserRatingType(el);
-
- let shouldProcess = true;
-
- // Filtering includesOnly
- if (config?.includesOnly?.length) {
- if (!config.includesOnly.some((include) => type === include)) {
- shouldProcess = false;
- }
- // Filter excludes
- } else if (config?.excludes?.length) {
- if (config.excludes.some((exclude) => type === exclude)) {
- shouldProcess = false;
- }
- }
-
- if (shouldProcess) {
- try {
- const item = this.buildUserRatings(el);
+ try {
+ const type = getUserRatingType(el);
+
+ let shouldProcess = true;
+
+ // Filtering includesOnly
+ if (config?.includesOnly?.length) {
+ if (!config.includesOnly.some((include) => type === include)) {
+ shouldProcess = false;
+ }
+ // Filter excludes
+ } else if (config?.excludes?.length) {
+ if (config.excludes.some((exclude) => type === exclude)) {
+ shouldProcess = false;
+ }
+ }
+
+ if (shouldProcess) {
+ const item = this.buildUserRatings(el, type);
const result = safeParse(CSFDUserRatingsSchema, item);
if (result.success) {
films.push(result.output as CSFDUserRatings);
} else {
console.warn(
`Skipping invalid user rating. Title: ${item.title}, ID: ${item.id}`,
JSON.stringify(flatten(result.issues))
);
}
- } catch (e) {
- console.warn(
- `Skipping user rating due to scraping error (DOM change?): ${(e as Error).message}`
- );
}
+ } catch (e) {
+ console.warn(
+ `Skipping user rating due to scraping error (DOM change?): ${(e as Error).message}`
+ );
}
}And update buildUserRatings to accept and reuse the already-extracted type:
- private buildUserRatings(el: HTMLElement): CSFDUserRatings {
+ private buildUserRatings(el: HTMLElement, type: string): CSFDUserRatings {
return {
id: getUserRatingId(el),
title: getUserRatingTitle(el),
year: getUserRatingYear(el),
- type: getUserRatingType(el),
+ type,
url: getUserRatingUrl(el),
colorRating: getUserRatingColorRating(el) as CSFDColorRating,
userDate: getUserRatingDate(el),
userRating: getUserRating(el) as CSFDStars
};
}🤖 Prompt for AI Agents
In `@src/services/user-ratings.service.ts` around lines 78 - 112,
getUserRatingType(el) is called outside the try/catch and can throw, aborting
the loop; move the extraction into the try block so DOM errors are caught,
capture the returned type and pass it into buildUserRatings to avoid
re-extraction, and update buildUserRatings' signature to accept the precomputed
type (then use that value instead of calling getUserRatingType again); keep the
existing validation flow with safeParse(CSFDUserRatingsSchema) and the existing
warning logs.
2 participants
Implemented strict runtime validation for User Ratings and User Reviews using Valibot.
Changes
valibot.src/dto/user-ratings.schema.tsandsrc/dto/user-reviews.schema.ts.src/services/user-ratings.service.tsandsrc/services/user-reviews.service.tsto validate scraped data.Skipping invalid user review...).getUserReviewTypewhere types without parentheses were being truncated (e.g., "TV film" becoming "V fil").CSFDUserReviewsinterface to allow nullableposter.Metrics & Report
Example
When the scraper encounters an invalid type (e.g., due to a DOM change or bug):
The item is skipped, and the rest of the list is returned correctly.
PR created automatically by Jules for task 3344119452736978508 started by @bartholomej
Summary by CodeRabbit
New Features
Bug Fixes
Chores