🛠️ DX: Support full URLs and slugs as input for API methods#187
🛠️ DX: Support full URLs and slugs as input for API methods#187bartholomej wants to merge 1 commit intomasterfrom
Conversation
Add `extractId` helper function to handle varied ID formats. Update API typings and signatures in `src/index.ts` to accept `number | string`. Refactor `movie`, `creator`, `userRatings`, and `userReviews` services to leverage `extractId`. Add corresponding unit tests to `tests/helpers.test.ts`. 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. |
📝 WalkthroughWalkthroughThis PR introduces flexible ID input support across the CSFD API. A new ChangesFlexible ID Input Support & Code Formatting Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 #187 +/- ##
==========================================
- Coverage 98.83% 98.36% -0.47%
==========================================
Files 34 34
Lines 771 794 +23
Branches 196 208 +12
==========================================
+ Hits 762 781 +19
- Misses 9 11 +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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/helpers/search-user.helper.ts (1)
8-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
getUserRealNamereturn type to includenull.The function returns
nullon multiple paths (Line 10, Line 15) but is typed asstring. This can break callers relying on the declared type.
Use: string | null(or return empty string consistently).As per coding guidelines, “Use specific types and avoid
anyin TypeScript.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helpers/search-user.helper.ts` around lines 8 - 17, Update the return type of getUserRealName to reflect that it can return null by changing its signature to return string | null (or alternately make all early returns return '' consistently), and ensure callers handle the nullable result; locate the function getUserRealName in src/helpers/search-user.helper.ts and adjust its signature and any dependent usages to accommodate the new nullable type.
🧹 Nitpick comments (1)
src/helpers/global.helper.ts (1)
48-61: ⚡ Quick winURL-parsing logic is duplicated from
parseIdFromUrl.The URL branch in
extractId(the reverse-loop + lang-prefix fallback) is identical to the body ofparseIdFromUrl. A future fix to one won't automatically apply to the other. The URL branch can delegate directly:♻️ Proposed simplification
- // If it's a URL - if (str.includes('/')) { - const parts = str.split('/'); - for (let i = parts.length - 1; i >= 0; i--) { - const p = parts[i]; - if (/^\d+-/.test(p)) { - return +p.split('-')[0] || null; - } - } - // Fallback logic for URL - const hasLangPrefix = LANG_PREFIX_REGEX.test(parts[1]); - const idSlug = parts[hasLangPrefix ? 3 : 2]; - const id = idSlug?.split('-')[0]; - return +id || null; - } + // If it's a URL + if (str.includes('/')) { + return parseIdFromUrl(str); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helpers/global.helper.ts` around lines 48 - 61, The URL-parsing branch in extractId duplicates the logic from parseIdFromUrl; replace the entire if (str.includes('/')) { ... } block in extractId with a direct delegation to parseIdFromUrl(str) (preserving the same return semantics, i.e., return the numeric id or null). Use the existing parseIdFromUrl function and keep references like LANG_PREFIX_REGEX untouched there, so extractId simply calls parseIdFromUrl(str) and returns its result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/user-ratings.service.ts`:
- Line 25: The line using a silent fallback ("const id = extractId(user) ??
user;") should be replaced with an explicit guard like in creator.service.ts and
movie.service.ts: call extractId(user), if it returns null/undefined throw an
Error (or return a handled error) rather than using the raw input; update the
code path in user-ratings.service.ts to ensure the function that currently
declares const id uses this guard so downstream logic always receives a
validated id (reference the extractId helper and the const id binding to locate
the code to change).
In `@src/services/user-reviews.service.ts`:
- Line 27: The current assignment const id = extractId(user) ?? user silently
uses the raw input when extractId(user) returns null; change this to throw a
descriptive error instead so invalid inputs fail fast (matching
creator.service.ts and movie.service.ts). Specifically, in
user-reviews.service.ts where id is computed (the code using extractId and the
id variable), check the result of extractId(user) and if it is null/undefined
throw a new Error with a clear message (e.g., "Invalid user id: <user>") rather
than falling back to the raw user string; ensure the thrown message mirrors the
style used in creator.service.ts/movie.service.ts for consistency.
In `@src/vars.ts`:
- Around line 39-44: The ternary checks in the URL builders treat 0 as falsy and
drop the page query; update the conditional in both userReviewsUrl and the
companion user ratings function (the lines building `${userUrl(user,
options)}/hodnoceni/${page ? '?page=' + page : ''}` and `${userUrl(user,
options)}/recenze/${page ? '?page=' + page : ''}`) to explicitly test for
presence (e.g., use `page !== undefined` or `page != null`) so page=0 is
included in the generated query string.
In `@test-dx-replace.ts`:
- Line 1: Delete the accidental scratch file test-dx-replace.ts entirely (it
only contains an invalid import of extractId) and remove any references to it in
the commit/exports; do not try to fix the import or enable
allowImportingTsExtensions—the project uses .js ESM imports, so the correct
action is to remove this debugging artifact and ensure no other modules import
test-dx-replace.ts or reference its import of extractId.
---
Outside diff comments:
In `@src/helpers/search-user.helper.ts`:
- Around line 8-17: Update the return type of getUserRealName to reflect that it
can return null by changing its signature to return string | null (or
alternately make all early returns return '' consistently), and ensure callers
handle the nullable result; locate the function getUserRealName in
src/helpers/search-user.helper.ts and adjust its signature and any dependent
usages to accommodate the new nullable type.
---
Nitpick comments:
In `@src/helpers/global.helper.ts`:
- Around line 48-61: The URL-parsing branch in extractId duplicates the logic
from parseIdFromUrl; replace the entire if (str.includes('/')) { ... } block in
extractId with a direct delegation to parseIdFromUrl(str) (preserving the same
return semantics, i.e., return the numeric id or null). Use the existing
parseIdFromUrl function and keep references like LANG_PREFIX_REGEX untouched
there, so extractId simply calls parseIdFromUrl(str) and returns its result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2353b54-a6e4-48f8-928d-5ad2abaec3b2
📒 Files selected for processing (16)
src/bin/export-reviews.tssrc/bin/lookup-movie.tssrc/bin/search.tssrc/bin/utils.tssrc/dto/options.tssrc/helpers/global.helper.tssrc/helpers/search-user.helper.tssrc/index.tssrc/services/creator.service.tssrc/services/movie.service.tssrc/services/user-ratings.service.tssrc/services/user-reviews.service.tssrc/types.tssrc/vars.tstest-dx-replace.tstests/helpers.test.ts
| config?: CSFDUserRatingConfig, | ||
| options?: CSFDOptions | ||
| ): Promise<CSFDUserRatings[]> { | ||
| const id = extractId(user) ?? user; |
There was a problem hiding this comment.
Same silent-fallback inconsistency as user-reviews.service.ts.
extractId(user) ?? user silently falls back to the raw string on invalid input rather than throwing early. Apply the same guard pattern used in creator.service.ts and movie.service.ts.
🛡️ Proposed fix
- const id = extractId(user) ?? user;
+ const id = extractId(user);
+ if (!id) {
+ throw new Error('node-csfd-api: user must be a valid number, slug, or URL');
+ }📝 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.
| const id = extractId(user) ?? user; | |
| const id = extractId(user); | |
| if (!id) { | |
| throw new Error('node-csfd-api: user must be a valid number, slug, or URL'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/user-ratings.service.ts` at line 25, The line using a silent
fallback ("const id = extractId(user) ?? user;") should be replaced with an
explicit guard like in creator.service.ts and movie.service.ts: call
extractId(user), if it returns null/undefined throw an Error (or return a
handled error) rather than using the raw input; update the code path in
user-ratings.service.ts to ensure the function that currently declares const id
uses this guard so downstream logic always receives a validated id (reference
the extractId helper and the const id binding to locate the code to change).
| config?: CSFDUserReviewsConfig, | ||
| options?: CSFDOptions | ||
| ): Promise<CSFDUserReviews[]> { | ||
| const id = extractId(user) ?? user; |
There was a problem hiding this comment.
Silent fallback on invalid input is inconsistent with other services.
extractId(user) ?? user silently proceeds with the raw string when extractId returns null (e.g., for 'blade-runner'), constructing a malformed URL and resulting in a confusing server error instead of an early validation failure. creator.service.ts and movie.service.ts both throw a descriptive error in this case — the same pattern should be applied here.
🛡️ Proposed fix to align with the throwing pattern
- const id = extractId(user) ?? user;
+ const id = extractId(user);
+ if (!id) {
+ throw new Error('node-csfd-api: user must be a valid number, slug, or URL');
+ }📝 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.
| const id = extractId(user) ?? user; | |
| const id = extractId(user); | |
| if (!id) { | |
| throw new Error('node-csfd-api: user must be a valid number, slug, or URL'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/user-reviews.service.ts` at line 27, The current assignment
const id = extractId(user) ?? user silently uses the raw input when
extractId(user) returns null; change this to throw a descriptive error instead
so invalid inputs fail fast (matching creator.service.ts and movie.service.ts).
Specifically, in user-reviews.service.ts where id is computed (the code using
extractId and the id variable), check the result of extractId(user) and if it is
null/undefined throw a new Error with a clear message (e.g., "Invalid user id:
<user>") rather than falling back to the raw user string; ensure the thrown
message mirrors the style used in creator.service.ts/movie.service.ts for
consistency.
| ): string => `${userUrl(user, options)}/hodnoceni/${page ? '?page=' + page : ''}`; | ||
| export const userReviewsUrl = ( | ||
| user: string | number, | ||
| page?: number, | ||
| options: Options = {} | ||
| ): string => `${userUrl(user, options)}/recenze/${page ? '?page=' + page : ''}`; |
There was a problem hiding this comment.
Use explicit page presence checks instead of truthiness.
On Line 39 and Line 44, page ? ... : '' treats 0 as “not provided.” Use page !== undefined (or page != null) to avoid silent query omission.
Proposed fix
export const userRatingsUrl = (
user: string | number,
page?: number,
options: Options = {}
-): string => `${userUrl(user, options)}/hodnoceni/${page ? '?page=' + page : ''}`;
+): string => `${userUrl(user, options)}/hodnoceni/${page !== undefined ? '?page=' + page : ''}`;
export const userReviewsUrl = (
user: string | number,
page?: number,
options: Options = {}
-): string => `${userUrl(user, options)}/recenze/${page ? '?page=' + page : ''}`;
+): string => `${userUrl(user, options)}/recenze/${page !== undefined ? '?page=' + page : ''}`;📝 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.
| ): string => `${userUrl(user, options)}/hodnoceni/${page ? '?page=' + page : ''}`; | |
| export const userReviewsUrl = ( | |
| user: string | number, | |
| page?: number, | |
| options: Options = {} | |
| ): string => `${userUrl(user, options)}/recenze/${page ? '?page=' + page : ''}`; | |
| ): string => `${userUrl(user, options)}/hodnoceni/${page !== undefined ? '?page=' + page : ''}`; | |
| export const userReviewsUrl = ( | |
| user: string | number, | |
| page?: number, | |
| options: Options = {} | |
| ): string => `${userUrl(user, options)}/recenze/${page !== undefined ? '?page=' + page : ''}`; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vars.ts` around lines 39 - 44, The ternary checks in the URL builders
treat 0 as falsy and drop the page query; update the conditional in both
userReviewsUrl and the companion user ratings function (the lines building
`${userUrl(user, options)}/hodnoceni/${page ? '?page=' + page : ''}` and
`${userUrl(user, options)}/recenze/${page ? '?page=' + page : ''}`) to
explicitly test for presence (e.g., use `page !== undefined` or `page != null`)
so page=0 is included in the generated query string.
| @@ -0,0 +1 @@ | |||
| import { extractId } from './src/helpers/global.helper.ts'; // Doesn't exist | |||
There was a problem hiding this comment.
Remove this accidentally committed scratch file.
test-dx-replace.ts is a one-line debugging artifact with a stale // Doesn't exist comment (the export now exists) and no functionality. It should not be shipped as part of this PR. Additionally, importing with a .ts extension requires allowImportingTsExtensions: true in tsconfig and cannot emit compiled output, which deviates from the project's established .js import convention for ESM.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-dx-replace.ts` at line 1, Delete the accidental scratch file
test-dx-replace.ts entirely (it only contains an invalid import of extractId)
and remove any references to it in the commit/exports; do not try to fix the
import or enable allowImportingTsExtensions—the project uses .js ESM imports, so
the correct action is to remove this debugging artifact and ensure no other
modules import test-dx-replace.ts or reference its import of extractId.
2 participants
💡 What
We've introduced a new helper function,
extractId, and updated the library's primary API methods (movie,creator,userRatings,userReviews) to natively support full CSFD URLs and URL slugs as input arguments, in addition to numeric IDs.🎯 Why
This drastically improves the Developer Experience (DX). Developers building tools on top of this library no longer need to write boilerplate code or regular expressions to extract numeric IDs from links before passing them into the API. They can just copy-paste a URL or a slug directly into the function call, and the library will figure it out automatically.
🚀 Examples
Before:
After:
PR created automatically by Jules for task 5973108370472887574 started by @bartholomej
Summary by CodeRabbit
New Features
Improvements