Conversation
Library services no longer output progress logs automatically — use the onProgress callback instead.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR introduces new CLI features (movie lookup, reviews export), adds a multi-platform installer script, extends the GitHub Actions workflow to build additional platform binaries (Linux ARM64, Windows x64), implements progress callbacks for paginated data fetching, refactors logging with a unified library prefix constant, adds background update checking, and updates documentation with installation instructions and CLI examples. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 Tip You can disable poems in the walkthrough.Disable the |
There was a problem hiding this comment.
Pull request overview
This PR expands the node-csfd-api project’s CLI with new commands (movie lookup, review export, update checks), introduces a standardized library log prefix, and updates packaging/release automation for additional binary targets.
Changes:
- Added CLI commands:
movie <id>,export reviews <userId>,update, and--version/-v, plus a background update check with cached GitHub release lookup. - Refactored CLI helpers into
src/bin/utils.ts(colors, CSV escaping, progress rendering) and switched library services from stdout progress logs to anonProgresscallback. - Updated packaging/release: new
binmappings inpackage.json, additional workflow artifacts (Linux ARM64, Windows x64, sha256sums), and a newinstall.sh.
Reviewed changes
Copilot reviewed 14 out of 19 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updates eslint-related dependency versions and lock entries. |
| tsdown.config.ts | Injects __VERSION__ define for CLI build; config changes for the CLI/server build. |
| src/vars.ts | Adds LIB_PREFIX constant for consistent library log prefixing. |
| src/services/user-reviews.service.ts | Replaces console progress logging with onProgress callback; uses LIB_PREFIX in warnings. |
| src/services/user-ratings.service.ts | Replaces console progress logging with onProgress callback; uses LIB_PREFIX in warnings. |
| src/services/movie.service.ts | Uses LIB_PREFIX for JSON-LD parsing error logs. |
| src/fetchers/index.ts | Uses LIB_PREFIX for fetch errors; improves trap warning text. |
| src/dto/user-reviews.ts | Adds onProgress to config interface docs/types. |
| src/dto/user-ratings.ts | Adds onProgress to config interface docs/types. |
| src/cli.ts | Major CLI update: new commands, version printing, update mechanism, background update hint, usage refresh. |
| src/bin/utils.ts | New shared CLI utilities: ANSI colors, CSV escaping, progress bar. |
| src/bin/lookup-movie.ts | New CLI handler for movie <id> output (pretty + --json). |
| src/bin/export-reviews.ts | New CLI handler for exporting reviews to CSV/JSON. |
| src/bin/export-ratings.ts | Uses shared utils and progress callback for rating export. |
| package.json | Version bump + exposes both csfd and node-csfd-api binaries. |
| install.sh | New install script for standalone binaries + checksum verification. |
| demo.ts | Updates example to show onProgress usage and allPages: true. |
| README.md | Reorders installation docs; documents new commands; adds onProgress option to tables. |
| .github/workflows/bin.yml | Builds/releases Linux ARM64 + Windows x64 artifacts and publishes sha256sums. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { distPackage } from 'rolldown-plugin-dist-package'; | ||
| import { defineConfig } from 'tsdown'; | ||
|
|
||
| const { version } = JSON.parse(readFileSync('./package.json', 'utf-8')) as { version: string }; |
There was a problem hiding this comment.
Reading ./package.json assumes the build always runs with repo root as the current working directory. Using a path resolved from import.meta.url (or process.cwd() + validation) would make the build config more robust across different invocations/CI setups.
| const { version } = JSON.parse(readFileSync('./package.json', 'utf-8')) as { version: string }; | |
| const { version } = JSON.parse(readFileSync(new URL('./package.json', import.meta.url), 'utf-8')) as { version: string }; |
| const noUpdateHintFor = new Set(['update']); | ||
| const updateHint = noUpdateHintFor.has(command) ? null : checkForUpdateInBackground(); |
There was a problem hiding this comment.
The background update hint can break machine-readable outputs (e.g., csfd movie <id> --json and csfd --version) by printing extra lines after the command output. Consider only showing the hint when process.stdout.isTTY is true and/or extending noUpdateHintFor to include --version, -v, and JSON-producing invocations.
| const noUpdateHintFor = new Set(['update']); | |
| const updateHint = noUpdateHintFor.has(command) ? null : checkForUpdateInBackground(); | |
| const noUpdateHintFor = new Set(['update', '--version', '-v']); | |
| const isJsonOutput = args.includes('--json'); | |
| const shouldShowUpdateHint = | |
| process.stdout.isTTY && !noUpdateHintFor.has(command) && !isJsonOutput; | |
| const updateHint = shouldShowUpdateHint ? checkForUpdateInBackground() : null; |
| if (va.major !== vb.major) return va.major - vb.major; | ||
| if (va.minor !== vb.minor) return va.minor - vb.minor; | ||
| if (va.patch !== vb.patch) return va.patch - vb.patch; | ||
| // pre-release < stable: 5.6.0-next.0 < 5.6.0 | ||
| if (va.pre && !vb.pre) return -1; | ||
| if (!va.pre && vb.pre) return 1; | ||
| return va.pre.localeCompare(vb.pre); |
There was a problem hiding this comment.
compareSemver() compares prerelease identifiers using plain localeCompare, which misorders numeric segments (e.g., 5.6.0-next.10 vs 5.6.0-next.9). This can cause incorrect update prompts; consider using a semver parser/comparator or implementing numeric comparison for dot-separated prerelease parts.
| if (va.major !== vb.major) return va.major - vb.major; | |
| if (va.minor !== vb.minor) return va.minor - vb.minor; | |
| if (va.patch !== vb.patch) return va.patch - vb.patch; | |
| // pre-release < stable: 5.6.0-next.0 < 5.6.0 | |
| if (va.pre && !vb.pre) return -1; | |
| if (!va.pre && vb.pre) return 1; | |
| return va.pre.localeCompare(vb.pre); | |
| const comparePrerelease = (aPre: string, bPre: string): number => { | |
| const aParts = aPre.split('.'); | |
| const bParts = bPre.split('.'); | |
| const len = Math.max(aParts.length, bParts.length); | |
| for (let i = 0; i < len; i++) { | |
| const aId = aParts[i]; | |
| const bId = bParts[i]; | |
| if (aId === undefined) return -1; | |
| if (bId === undefined) return 1; | |
| const aNum = /^\d+$/.test(aId) ? Number(aId) : NaN; | |
| const bNum = /^\d+$/.test(bId) ? Number(bId) : NaN; | |
| const aIsNum = !Number.isNaN(aNum); | |
| const bIsNum = !Number.isNaN(bNum); | |
| if (aIsNum && bIsNum) { | |
| if (aNum !== bNum) return aNum - bNum; | |
| } else if (aIsNum && !bIsNum) { | |
| // Numeric identifiers have lower precedence than non-numeric | |
| return -1; | |
| } else if (!aIsNum && bIsNum) { | |
| return 1; | |
| } else { | |
| const cmp = aId.localeCompare(bId); | |
| if (cmp !== 0) return cmp; | |
| } | |
| } | |
| return 0; | |
| }; | |
| if (va.major !== vb.major) return va.major - vb.major; | |
| if (va.minor !== vb.minor) return va.minor - vb.minor; | |
| if (va.patch !== vb.patch) return va.patch - vb.patch; | |
| // pre-release < stable: 5.6.0-next.0 < 5.6.0 | |
| if (va.pre && !vb.pre) return -1; | |
| if (!va.pre && vb.pre) return 1; | |
| return comparePrerelease(va.pre, vb.pre); |
| @@ -30,6 +33,9 @@ export default defineConfig([ | |||
| 'bin/export-ratings': './src/bin/export-ratings.ts', | |||
There was a problem hiding this comment.
The CLI dynamically imports ./bin/export-reviews and ./bin/lookup-movie, but they are not included in the tsdown entry map. Given the current pattern (explicit bin/* entries), this likely means those modules won’t be emitted to dist/ and the CLI will fail at runtime. Add them to entry (and any other new bin modules) so they’re built/published.
| 'bin/export-ratings': './src/bin/export-ratings.ts', | |
| 'bin/export-ratings': './src/bin/export-ratings.ts', | |
| 'bin/export-reviews': './src/bin/export-reviews.ts', | |
| 'bin/lookup-movie': './src/bin/lookup-movie.ts', |
| if (config?.allPages) { | ||
| console.log('User', user, url); | ||
| console.log('Fetching all pages', pages); | ||
| for (let i = 2; i <= pages; i++) { | ||
| console.log('Fetching page', i, 'out of', pages, '...'); | ||
| config.onProgress?.(i, pages); | ||
| const url = userReviewsUrl(user, i, { language: options?.language }); |
There was a problem hiding this comment.
onProgress is only invoked for pages 2..N, so progress consumers never get a callback for the first fetched page (and never get called at all when pages === 1). If the intent is “on each page fetch”, consider calling config.onProgress?.(pageToFetch, pages) after the initial fetch (or onProgress?.(1, pages) when starting from page 1).
| @@ -40,10 +40,8 @@ export class UserReviewsScraper { | |||
| allReviews = this.getPage(config, reviews); | |||
|
|
|||
| if (config?.allPages) { | |||
There was a problem hiding this comment.
There are existing Vitest suites for UserReviewsScraper, but none validate the new onProgress callback behavior. Adding a mocked fetchPage test that asserts onProgress is called with expected (page,total) values would prevent regressions (especially around 1-page users and multi-page loops).
| if (config?.allPages) { | |
| if (config?.allPages) { | |
| config.onProgress?.(1, pages); |
| if curl -fsSL "$CHECKSUMS_URL" -o "$TMP_DIR/sha256sums.txt" 2>/dev/null; then | ||
| EXPECTED=$(grep "csfd-$TARGET.tar.gz" "$TMP_DIR/sha256sums.txt" | awk '{print $1}') | ||
| if command -v sha256sum > /dev/null 2>&1; then | ||
| ACTUAL=$(sha256sum "$ARCHIVE_PATH" | awk '{print $1}') |
There was a problem hiding this comment.
Because the script runs with set -e, the grep ... | awk ... pipeline used to extract the expected checksum will cause the installer to exit if the checksum line isn’t found (e.g., missing/renamed asset in a release). Consider handling “checksum entry not found” explicitly and falling back to the “skip verification” path instead of aborting unexpectedly.
| function getCommandName(): string { | ||
| const scriptPath = process.argv[1] ?? ''; | ||
| const basename = scriptPath.split('/').pop() ?? ''; | ||
| if (basename === 'csfd' || basename === 'node-csfd-api') return basename; | ||
| if (scriptPath.includes('node-csfd-api')) return 'npx node-csfd-api'; | ||
| return 'csfd'; |
There was a problem hiding this comment.
getCommandName() derives the basename by splitting on '/', which won’t work on Windows paths (\). Use node:path (e.g., basename(process.argv[1] ?? '')) or normalize separators so Windows users get correct command names/instructions.
| const controller = new AbortController(); | ||
| const timer = setTimeout(() => controller.abort(), timeoutMs); | ||
| try { | ||
| const res = await fetch(GITHUB_API_LATEST, { signal: controller.signal }); |
There was a problem hiding this comment.
fetchLatestVersion() doesn’t check res.ok before parsing JSON. When GitHub returns non-2xx (rate limit, abuse detection, etc.), this will silently yield an empty version and make csfd update report “Could not determine latest version.” Consider handling non-OK responses explicitly (e.g., throw with status/message or fall back to cached value).
| const res = await fetch(GITHUB_API_LATEST, { signal: controller.signal }); | |
| const res = await fetch(GITHUB_API_LATEST, { signal: controller.signal }); | |
| if (!res.ok) { | |
| throw new Error(`GitHub API request failed with status ${res.status} ${res.statusText}`); | |
| } |
| | `exclude` | `CSFDFilmTypes[]` | `null` | Exclude specific content types (e.g., `['episode']`) | | ||
| | `allPages` | `boolean` | `false` | Fetch all pages of ratings | | ||
| | `allPagesDelay` | `number` | `0` | Delay between page requests in milliseconds | | ||
| | `page` | `number` | `1` | Fetch specific page number | | ||
| | `onProgress` | `(page: number, total: number) => void` | `undefined` | Called on each page fetch — use for progress bars or logging | | ||
|
|
||
| > 📝 **Note**: `includesOnly` and `exclude` are mutually exclusive. If both are provided, `includesOnly` takes precedence. |
There was a problem hiding this comment.
The option name is documented as exclude, but the actual config field in the code is excludes (plural) (CSFDUserRatingConfig.excludes). This mismatch will confuse users and the note about mutual exclusivity should also reference excludes.
| | `exclude` | `CSFDFilmTypes[]` | `null` | Exclude specific content types (e.g., `['episode']`) | | |
| | `allPages` | `boolean` | `false` | Fetch all pages of ratings | | |
| | `allPagesDelay` | `number` | `0` | Delay between page requests in milliseconds | | |
| | `page` | `number` | `1` | Fetch specific page number | | |
| | `onProgress` | `(page: number, total: number) => void` | `undefined` | Called on each page fetch — use for progress bars or logging | | |
| > 📝 **Note**: `includesOnly` and `exclude` are mutually exclusive. If both are provided, `includesOnly` takes precedence. | |
| | `excludes` | `CSFDFilmTypes[]` | `null` | Exclude specific content types (e.g., `['episode']`) | | |
| | `allPages` | `boolean` | `false` | Fetch all pages of ratings | | |
| | `allPagesDelay` | `number` | `0` | Delay between page requests in milliseconds | | |
| | `page` | `number` | `1` | Fetch specific page number | | |
| | `onProgress` | `(page: number, total: number) => void` | `undefined` | Called on each page fetch — use for progress bars or logging | | |
| > 📝 **Note**: `includesOnly` and `excludes` are mutually exclusive. If both are provided, `includesOnly` takes precedence. |
What's Changed
New CLI Commands
csfd movie <id>— display movie details (title, rating, genres, cast, VOD) directly in the terminal;--jsonflag for pipe-friendly raw outputcsfd export reviews <userId>— export user reviews to CSV (default) or JSONcsfd update— check for a new version with context-aware upgrade instructions (detects npx / Homebrew / binary / Windows)csfd --version/-v— print current versionAutomatic Update Check
The CLI now checks for updates in the background (cached for 24 hours, 3s timeout) and shows a hint at the end of command output when a newer version is available. No impact
on command performance.
New Binary Targets
csfd-windows-x64.zip) — standalone.exe, no runtime requiredcsfd-linux-arm64.tar.gz) — covers the increasingly common ARM64 Linux setup (VMs, cloud, Raspberry Pi)sha256sums.txt) included in every release for verificationBreaking:
onProgressreplaces console output in library servicesuserRatings()anduserReviews()no longer print progress to stdout. Use the newonProgresscallback instead:Other
[node-csfd-api]prefix on all library warnings and errors; extracted asLIB_PREFIXconstantbinfield inpackage.jsonnow exposes bothcsfdandnode-csfd-apicommandscolors,escapeCsvField,renderProgress) extracted tosrc/bin/utils.ts;parseNumericArgeliminates repeated argument validationSummary by CodeRabbit
Release Notes
New Features
moviesubcommand to look up and display movie detailsexport reviewssubcommand to export user reviews in JSON or CSV formatImprovements
csfdcommand name alongsidenode-csfd-apiDocumentation