Add --verbose flag, progress indicator, and improve documentation
#93
Conversation
Introduced a `--verbose` option to the CLI for displaying detailed processing information, including file statistics and configuration. This enhancement improves debugging and gives users greater visibility into the minification process. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Added various tests to verify the behavior of the `--verbose` flag in different scenarios, including single file, directory, and standard input processing. These tests ensure comprehensive coverage and validate expected verbose outputs. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Rearranged sections in the README for better clarity and logical flow. Added examples for `--verbose` mode and clarified CLI usage with updated commands, ensuring consistency and improving usability for users. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Updated `version` field in `package.json` and `package-lock.json` to reflect the transition from 3.1.0 to 3.2.0. This change prepares the project for the new release. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Added details about the `--verbose` flag, README restructuring, and other updates introduced with version 3.2.0. This provides users with a comprehensive summary of recent changes and improvements. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
This adds a test to ensure the CLI correctly handles the --version flag, returning the version number and no errors. The test verifies both the exit code and expected output format. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Add version output option to README in "html-minifier-next". Refine phrasing in "on-web-development-2" manuscript entries for improved clarity and accuracy. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Added details about the `--version`/`-V` flag to the changelog, documenting its functionality and associated tests. This ensures users are informed about the new feature and its purpose. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Refined the README to clarify option usage in both config files and CLI. Enhanced formatting to list both camelCase and kebab-case representations for better clarity and usability. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Rephrased sections for better clarity and consistency, merging redundant content and improving flow. Adjusted formatting and fixed minor grammatical issues to enhance readability. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Added note to the changelog about making CLI options more explicit in the documentation. This ensures users are informed about the changes for better clarity and usability. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Introduced a progress bar for directory processing in CLI to enhance user feedback. Includes counting total files, updating the progress bar in real-time, and clearing the progress display upon completion. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Introduced tests to validate CLI progress indicator behavior in various scenarios, including non-TTY environments, verbose mode, dry runs, and nested directories. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Added details about the new automatic progress indicator for directory processing in CLI. This update ensures the changelog reflects recent enhancements for user feedback and interactivity. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
WalkthroughAdds a verbose flag and dry-run mode with per-file statistics and TTY-aware progress; threads verbose/progress through processing functions; implements robust config loading (JSON → CJS → ESM) and normalization; updates README and CHANGELOG; bumps package to 3.2.0 and adds extensive CLI tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config as ConfigLoader
participant Counter as ProgressCounter
participant Proc as Processor
participant FS as FileSystem
User->>CLI: run minify --verbose --input src --output out
CLI->>Config: loadConfigFromPath(path) (try JSON → CJS → ESM)
Config-->>CLI: normalized config
CLI->>Counter: countFiles(src, extensions, skipRootAbs)
Counter-->>CLI: totalCount
CLI->>Proc: processDirectory(src, out, ext, isDry=false, isVerbose=true, skipRootAbs, progress)
loop for each file
Proc->>Counter: updateProgress(currentIndex, totalCount)
alt TTY & progress enabled
Counter-->>User: render progress + current file
end
Proc->>FS: read file
Proc->>Proc: minify content
alt not dry-run
Proc->>FS: write output
end
alt isVerbose or isDry
Proc-->>User: emit per-file stats and active options
end
end
Proc-->>User: overall summary (if verbose or dry)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
README.md (2)
35-36: Keep short flags comma-separated for consistencyIn the CLI options table, prefer “-o, --output”, “-v, --verbose”, “-V, --version” for parity with Commander’s help text.
-| `-o --output <file>` | Specify output file ... +| `-o, --output <file>` | Specify output file ... -| `-v --verbose` | Show detailed processing information ... +| `-v, --verbose` | Show detailed processing information ... -| `-V --version` | Output the version number ... +| `-V, --version` | Output the version number ...Also applies to: 39-40
208-212: Clarify impact of processing “all files”Add a bold caution that non-HTML/binary files will be read as UTF‑8 and passed to the minifier; recommend always setting --file-ext in directory mode.
CHANGELOG.md (1)
7-14: Changelog entry reads wellAdd a brief “CLI UX” bullet mentioning progress bar TTY-only behavior for discoverability.
cli.js (6)
110-110: Use explicit radix with parseIntAvoid locale/format surprises; pass 10 to parseInt.
- customFragmentQuantifierLimit: ['Set maximum quantifier limit for custom fragments to prevent ReDoS attacks (default: 200)', parseInt], + customFragmentQuantifierLimit: ['Set maximum quantifier limit for custom fragments to prevent ReDoS attacks (default: 200)', (v) => parseInt(v, 10)], ... - maxInputLength: ['Maximum input length to prevent ReDoS attacks', parseInt], - maxLineLength: ['Specify a maximum line length; compressed output will be split by newlines at valid HTML split-points', parseInt], + maxInputLength: ['Maximum input length to prevent ReDoS attacks', (v) => parseInt(v, 10)], + maxLineLength: ['Specify a maximum line length; compressed output will be split by newlines at valid HTML split-points', (v) => parseInt(v, 10)],Also applies to: 125-127
369-374: Progress should advance after successful processingIncrementing before processing can show 100% while the last file is still running. Update after processFile resolves.
- // Update progress before processing - if (progress) { - progress.current++; - updateProgress(progress.current, progress.total, inputFile); - } ... - const fileStats = await processFile(inputFile, outputFile, isDryRun, isVerbose); + const fileStats = await processFile(inputFile, outputFile, isDryRun, isVerbose); + if (progress) { + progress.current++; + updateProgress(progress.current, progress.total, inputFile); + }Also applies to: 380-383
177-206: Support ESM config files as a fallbackCurrently only JSON or CJS via require(). Add dynamic import() for ESM configs (.mjs or “type”: “module” users).
try { config = JSON.parse(data); } catch (je) { - try { - config = require(path.resolve(configPath)); - } catch (ne) { - fatal('Cannot read the specified config file.\nAs JSON: ' + je.message + '\nAs module: ' + ne.message); - } + const abs = path.resolve(configPath); + try { + config = require(abs); + } catch (ne) { + try { + // ESM fallback + const mod = await import(pathToFileURL(abs).href); + config = mod.default || mod; + } catch (ee) { + fatal('Cannot read the specified config file.\nAs JSON: ' + je.message + '\nAs CJS: ' + ne.message + '\nAs ESM: ' + ee.message); + } + } }Note: add
import { pathToFileURL } from 'url';at top.
393-399: Show explicitly provided “disabled” optionsThe “Options:” list filters truthy values, so flags explicitly set to false (e.g.,
--no-html5) won’t appear. Consider listing options provided by the user regardless of truthiness.-const activeOptions = Object.keys(minifierOptions).filter(key => minifierOptions[key]); +const activeOptions = Object.entries(minifierOptions) + .filter(([k, _]) => program.getOptionValueSource(k === 'minifyURLs' ? 'minifyUrls' : camelCase(k)) === 'cli') + .map(([k, v]) => (typeof v === 'boolean' ? (v ? k : `no-${k}`) : k));Also applies to: 468-474
260-265: Return value consistency
processFilereturns stats in dry/verbose modes, null otherwise. This is fine, but consider always returning stats and deciding on printing separately to simplify aggregation.
496-506: Avoid double counting files on rename/move during traversal
countFilescomputes before traversal; concurrent changes can desync totals. Not critical, but consider toleratingcurrent > totalinupdateProgressto avoid negative bar width.- const filled = Math.floor((current / total) * barWidth); + const ratio = total ? Math.min(current / total, 1) : 0; + const filled = Math.floor(ratio * barWidth);Also applies to: 508-514
tests/cli.spec.js (3)
792-810: Add a test for output-dir under input-dir (skip traversal)Covers the
skipRootAbslogic and prevents regressions.+ test('should skip traversing into output directory when nested in input directory', async () => { + await fs.promises.mkdir(path.resolve(fixturesDir, 'tmp/in/sub'), { recursive: true }); + await fs.promises.writeFile(path.resolve(fixturesDir, 'tmp/in/a.html'), '<html><body>a</body></html>'); + const result = execCliWithStderr([ + '--input-dir=tmp/in', + '--output-dir=tmp/in/sub', // nested + '--collapse-whitespace' + ]); + assert.strictEqual(result.exitCode, 0); + // Should write only to sub/, and must not reprocess files it just wrote + assert.strictEqual(existsFixture('tmp/in/sub/a.html'), true); + });
858-878: Add a symlink skip testValidates
lstat().isSymbolicLink()behavior.+ test('should skip symbolic links', async () => { + await fs.promises.mkdir(path.resolve(fixturesDir, 'tmp'), { recursive: true }); + await fs.promises.writeFile(path.resolve(fixturesDir, 'tmp/real.html'), '<html><body>x</body></html>'); + // Create symlink pointing to real.html + const target = path.resolve(fixturesDir, 'tmp/real.html'); + const link = path.resolve(fixturesDir, 'tmp/link.html'); + try { await fs.promises.symlink(target, link); } catch {} + const result = execCliWithStderr([ + '--input-dir=tmp', + '--output-dir=tmp-out', + '--collapse-whitespace' + ]); + assert.strictEqual(result.exitCode, 0); + // Only real file should be processed + assert.strictEqual(existsFixture('tmp-out/real.html'), true); + assert.strictEqual(existsFixture('tmp-out/link.html'), false); + await removeFixture('tmp-out'); + });
505-523: Consider EPIPE test for STDOUT pipingEnsures graceful exit when downstream closes early (e.g.,
| head -n1).+ test('should handle EPIPE gracefully when piping to head', () => { + const { status, stderr } = spawnSync('bash', ['-lc', `node "${cliPath}" --collapse-whitespace < default.html | head -n1`], { + cwd: fixturesDir + }); + // Exit code should be 0 and no noisy errors + assert.strictEqual(status, 0); + assert.strictEqual(stderr.toString().trim(), ''); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)README.md(6 hunks)cli.js(10 hunks)package.json(1 hunks)tests/cli.spec.js(1 hunks)
🔇 Additional comments (1)
package.json (1)
81-81: Version bump LGTM3.2.0 aligns with new CLI features and docs. No issues.
Revised CLI tests to use `path.join` for asserting file paths, ensuring compatibility across different environments. This change addresses potential issues with hardcoded directory separators. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Resolved a minor alignment issue in the README’s options table for better consistency. This change ensures uniform formatting across all rows. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Updated CLI options in the README to use consistent formatting for short and long flags, improving clarity. Adjusted notes and examples for better readability and user understanding. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Combined the npm version and build status badges into a single line in the README. This change ensures a more concise and visually cleaner layout. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Enhanced CLI for better config file handling, including ESM support, and standardized integer parsing across options. Improved progress tracking accuracy and moved updates after file processing for better user feedback. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Documented the addition of ESM config file support and improved verbose mode details. Also noted internal improvements like return value consistency and README restructuring. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Added tests to ensure the CLI handles EPIPE errors gracefully when piping, skips processing symbolic links, and avoids traversing nested output directories. These additions improve test coverage and ensure robust behavior in edge cases. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Documented the addition of CLI tests, edge case handling, and minifier validations, including symbolic links, nested output directories, and pipe errors. These updates ensure the changelog reflects recent internal test enhancements. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
README.md (1)
148-148: The table formatting issue has been fixed.The previous review flagged a missing space before the second pipe in this table row. The current code shows proper spacing, so this issue has been resolved.
🧹 Nitpick comments (1)
cli.js (1)
403-410: Consider extracting duplicate options display logic.The options display logic appears twice with identical implementation (lines 403-410 and 481-486). This duplication could be reduced by extracting it into a helper function.
Apply this refactor to reduce duplication:
+function getActiveOptionsDisplay(minifierOptions, program) { + const activeOptions = Object.entries(minifierOptions) + .filter(([k]) => program.getOptionValueSource(k === 'minifyURLs' ? 'minifyUrls' : camelCase(k)) === 'cli') + .map(([k, v]) => (typeof v === 'boolean' ? (v ? k : `no-${k}`) : k)); + if (activeOptions.length > 0) { + console.error('Options: ' + activeOptions.join(', ')); + } +} + const writeMinify = async () => { const minifierOptions = createOptions(); // Show config info if verbose if (programOptions.verbose || programOptions.dry) { - const activeOptions = Object.entries(minifierOptions) - .filter(([k]) => program.getOptionValueSource(k === 'minifyURLs' ? 'minifyUrls' : camelCase(k)) === 'cli') - .map(([k, v]) => (typeof v === 'boolean' ? (v ? k : `no-${k}`) : k)); - if (activeOptions.length > 0) { - console.error('Options: ' + activeOptions.join(', ')); - } + getActiveOptionsDisplay(minifierOptions, program); }And similarly for the directory processing section at lines 479-487.
Also applies to: 481-486
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)README.md(7 hunks)cli.js(15 hunks)tests/cli.spec.js(2 hunks)
⏰ 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: Node current (macos-latest)
🔇 Additional comments (10)
README.md (1)
23-241: Excellent documentation improvements!The restructured documentation significantly enhances clarity and usability:
- Comprehensive CLI options table with practical examples
- Clear configuration file guidance for both JSON and JavaScript modules
- Detailed options quick reference covering all minifier settings
- Rich examples section demonstrating verbose mode, dry-run, and various CLI patterns
The reorganization makes it much easier for users to find the information they need.
cli.js (4)
179-196: Well-implemented ESM config fallback!The cascading fallback from JSON → CJS → ESM provides excellent flexibility for different project configurations. The use of
pathToFileURLensures cross-platform compatibility, and the comprehensive error message helps users troubleshoot config loading issues.
293-338: Progress tracking implementation looks solid!The progress utilities are well-designed:
- Recursive file counting properly skips output directories and symlinks
- Progress bar provides clear visual feedback with percentage and current file
- Path truncation (50 chars) prevents display overflow
- Uses stderr appropriately for non-data output
The implementation handles the necessary edge cases effectively.
241-262: Verbose mode implementation is comprehensive and user-friendly!The verbose mode is well-integrated:
- Automatically enabled with
--dryfor better user experience- Shows active CLI options at the start of processing
- Provides per-file statistics with relative paths for readability
- Uses stderr appropriately to keep stdout clean for piped output
- Filters options display to show only explicitly provided CLI flags (not config-sourced options)
The implementation is consistent across single-file, directory, and STDIN processing modes.
Also applies to: 403-410, 475-487
340-397: Directory processing with progress tracking is well-implemented!The implementation handles several important scenarios correctly:
- Prevents reprocessing output directory when nested in input (skipRootAbs)
- Skips symbolic links to avoid potential issues
- Threads isVerbose and progress parameters through recursive calls
- Progress indicator disabled appropriately in non-TTY environments and verbose mode
- Progress cleared cleanly after completion
The logic is sound and edge cases are properly handled.
Also applies to: 508-527
tests/cli.spec.js (4)
555-563: LGTM! EPIPE test properly validates graceful handling.The test correctly simulates piping output to
headwhich closes the pipe early, and verifies that the CLI handles this gracefully without errors. This is important for composability with other Unix tools.
665-792: Excellent verbose mode test coverage!The verbose mode tests are comprehensive and well-structured:
- Tests all input modes (single file, directory, STDIN)
- Verifies output appears in stderr, not stdout
- Tests automatic verbose enablement with
--dry- Tests explicit
--verboseflag- Includes negative test to ensure no verbose output without the flag
- Validates file creation and exit codes
The test suite provides strong confidence in the verbose mode implementation.
802-866: Progress indicator tests are thorough and well-designed!The tests properly validate that the progress indicator:
- Does not appear in non-TTY environments (CI/piped contexts)
- Is disabled when using
--verbose(replaced by per-file stats)- Is disabled when using
--dry(replaced by per-file stats)- Uses negative assertions to confirm absence of progress bar
The tests ensure the progress indicator doesn't interfere with other output modes.
868-931: Edge case tests provide excellent coverage!These tests validate important scenarios:
- Nested subdirectories: ensures recursive traversal works correctly
- Output directory skip: prevents infinite loops when output is nested in input
- Symbolic link skip: avoids following symlinks, with proper Windows fallback
The tests demonstrate good attention to real-world edge cases and cross-platform compatibility.
CHANGELOG.md (1)
7-26: Changelog accurately documents all changes!The version 3.2.0 changelog entry is well-organized and comprehensive:
- Added: Clearly documents new verbose flag, progress indicator, ESM config support, and version flag tests
- Changed: Notes documentation improvements and verbose mode behavior
- Internal: Captures code quality improvements and test coverage expansion
The entries align perfectly with the actual code changes in cli.js, tests/cli.spec.js, and README.md. The categorization follows Keep a Changelog format appropriately.
Moved verbose mode’s active options display logic into a reusable `getActiveOptionsDisplay` helper function. This change improves code readability and minimizes duplication by consolidating repeated logic. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cli.js (2)
302-331: Consider logging skipped files in verbose mode.The function correctly counts files for the progress indicator, but silently catches errors at lines 305, 312, and 318. While this is acceptable for a non-critical progress counter, consider logging skipped files/directories (due to permissions or other errors) when verbose mode is enabled.
For example, at line 305:
- const files = await fs.promises.readdir(dir).catch(() => []); + const files = await fs.promises.readdir(dir).catch((err) => { + if (isVerbose) { + console.error(` Warning: Cannot read directory ${dir}: ${err.message}`); + } + return []; + });
333-347: LGTM! Well-implemented progress indicator.The progress bar correctly uses Unicode characters and ANSI escape sequences, with appropriate checks for TTY environments. The path truncation at line 340 provides a good user experience.
Optional: Consider extracting the magic number
50at line 340 as a named constant for clarity:const MAX_DISPLAY_PATH_LENGTH = 50; // ... const displayPath = relativePath.length > MAX_DISPLAY_PATH_LENGTH ? '...' + relativePath.slice(-(MAX_DISPLAY_PATH_LENGTH - 3)) : relativePath;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cli.js(15 hunks)
🔇 Additional comments (9)
cli.js (9)
30-30: LGTM! Correct import for ESM config loading.The
pathToFileURLimport is properly used at line 190 to convert absolute file paths to file URLs for ESM dynamic imports, which is necessary for cross-platform compatibility (especially Windows).
110-110: LGTM! Numeric option parsers are correctly implemented.The parser functions correctly use
parseInt(v, 10)to convert string values to integers with explicit base-10 radix.Also applies to: 126-127
167-167: LGTM! Clean verbose flag definition.The verbose flag is properly defined following the commander.js pattern and integrates well with the rest of the CLI options.
179-214: Excellent ESM config loading implementation.The three-stage fallback (JSON → CJS → ESM) is well-structured and handles the cross-platform requirements correctly. Using
pathToFileURL(abs).hrefat line 190 properly converts absolute paths to file URLs for ESM dynamic imports. The comprehensive error message at line 193 provides helpful debugging information.
241-248: LGTM! Clean option display helper.The function correctly filters CLI-provided options and formats them appropriately. The boolean formatting with
no-prefix and stderr output are both proper choices.
250-282: LGTM! Well-integrated verbose and dry-run support.The function correctly calculates and displays statistics in both verbose and dry-run modes. The use of
path.relative()for user-friendly output and the comprehensive stats formatting enhance the user experience.
349-406: LGTM! Proper parameter threading and progress tracking.The function correctly threads
isVerboseandprogressparameters through recursive calls and updates progress after each file. The extensions normalization at lines 351-353 avoids redundant parsing.
408-463: LGTM! Clean verbose and dry-run integration.The function properly displays configuration, calculates statistics, and handles both verbose and dry-run modes. The use of stderr for informational output is correct.
478-537: Excellent integration of progress and verbose features.The logic correctly handles the interaction between verbose, dry, and progress modes:
- Dry mode enables verbose (line 480) for better user feedback
- Progress is only shown in TTY when not verbose (line 508) to avoid output conflicts
- File counting before processing (lines 513-514) provides accurate progress percentage
- Stats totals (lines 527-536) provide useful summary information
For very large directory trees (e.g., 100K+ files), the upfront counting at lines 513-514 might introduce a noticeable delay. Consider adding a brief message if counting takes more than a second, or falling back to an indeterminate progress indicator. However, this is a minor UX consideration rather than a functional issue.
Replaced hardcoded path length limit with a `maxPathLength` constant, enhancing code maintainability. Updated progress display to dynamically calculate truncation limits for better readability. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cli.js (2)
241-248: Consider extracting boolean option formatting.The function correctly identifies CLI options and formats them for display. For slightly improved readability, consider extracting the boolean formatting logic:
function getActiveOptionsDisplay(minifierOptions) { + const formatOption = ([k, v]) => { + if (typeof v === 'boolean') return v ? k : `no-${k}`; + return k; + }; const activeOptions = Object.entries(minifierOptions) .filter(([k]) => program.getOptionValueSource(k === 'minifyURLs' ? 'minifyUrls' : camelCase(k)) === 'cli') - .map(([k, v]) => (typeof v === 'boolean' ? (v ? k : `no-${k}`) : k)); + .map(formatOption); if (activeOptions.length > 0) { console.error('Options: ' + activeOptions.join(', ')); } }
250-281: Consider extracting stats calculation helper.The per-file stats and verbose output work correctly. However, the stats calculation and formatting (lines 262-270) are duplicated in
writeMinify(lines 427-431).Extract a helper to reduce duplication:
function calculateStats(original, minified) { const originalSize = Buffer.byteLength(original, 'utf8'); const minifiedSize = Buffer.byteLength(minified, 'utf8'); const saved = originalSize - minifiedSize; const sign = saved >= 0 ? '-' : '+'; const percentage = originalSize ? ((Math.abs(saved) / originalSize) * 100).toFixed(1) : '0.0'; return { originalSize, minifiedSize, saved, sign, percentage }; }Then use in both locations:
const stats = calculateStats(data, minified); if (isDryRun || isVerbose) { console.error(` ✓ ${path.relative(process.cwd(), inputFile)}: ${stats.originalSize.toLocaleString()} → ${stats.minifiedSize.toLocaleString()} bytes (${stats.sign}${Math.abs(stats.saved).toLocaleString()}, ${stats.percentage}%)`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cli.js(15 hunks)
🔇 Additional comments (4)
cli.js (4)
179-194: LGTM! ESM config fallback correctly implemented.The progressive fallback (JSON → CJS → ESM) is well-structured. Using
pathToFileURL(abs).hrefensures cross-platform compatibility, especially for Windows paths. The comprehensive error message helpfully reports all three failure modes.
333-350: LGTM! Progress indicator handles edge cases correctly.The implementation properly handles zero-total scenarios (line 335), truncates long paths for readability (lines 341-343), and uses standard ANSI escape sequences. The progress bar provides good user feedback without cluttering the output.
510-521: LGTM! Progress indicator setup is well-designed.The logic correctly avoids progress bars when output is not a TTY or when verbose mode shows per-file stats (line 511). Pre-counting files (line 517) enables accurate progress percentages. The conditional initialization prevents unnecessary work when no files match.
482-489: LGTM! Verbose mode integration is intuitive.Automatically enabling verbose output for dry-run mode (line 483) aligns with user expectations—dry runs are for inspection, so showing detailed info makes sense. Displaying active config options (lines 486-489) aids debugging and confirms what settings are in effect.
Introduced a `calculateStats` utility function to standardize size calculation and improve code readability. Updated relevant operations to use the new function, reducing duplication and potential errors. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Added tests to ensure the CLI throws errors for invalid numeric values in options like max-line-length, max-input-length, and custom-fragment-quantifier-limit. These tests enhance validation coverage and safeguard against incorrect CLI usage. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Added details about the fix for numeric option validation, improving clarity around recent changes. This ensures users are informed about stricter validation and improved error messaging. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cli.js (1)
315-344: Silent error handling may hide filesystem issues.The function silently ignores errors from
readdir,realpath, andlstatoperations. While this is used only for progress display (non-critical), silent failures can mask permission errors or inaccessible paths, leading to misleading progress indicators.Consider logging warnings for unexpected failures as noted in previous reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)cli.js(14 hunks)tests/cli.spec.js(3 hunks)
⏰ 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: Node lts/* (windows-latest)
🔇 Additional comments (16)
CHANGELOG.md (2)
23-23: Verify numeric validation error messaging matches implementation.Line 23 claims numeric option validation now "errors on invalid input (e.g.,
--max-line-length=abc) with clear error message instead of silently passingNaN."Please confirm:
- The error message is user-friendly and actionable (not just a generic validation error)
- All numeric options are covered (e.g.,
--max-line-length, any others?)- This applies to all numeric CLI flags
28-28: Verify processFile return consistency across all code paths.Line 28 states "
processFilenow always returns stats object." This is a behavioral change that affects downstream code.Please verify:
- All code paths in
processFilereturn a stats object (no early returns or error paths that skip this)- All callers of
processFilehave been updated to handle the stats object consistently- Tests validate this return-value consistency
tests/cli.spec.js (5)
109-134: LGTM!These tests properly validate the numeric input validation for CLI options, addressing the requirement from previous reviews to prevent NaN values from being passed to the minifier.
582-590: LGTM!This test properly validates the EPIPE error handling when piping to commands like
head, ensuring graceful exit without noisy errors.
691-820: LGTM!Comprehensive test coverage for verbose mode across different input scenarios (single file, directory, STDIN) and flag combinations. The tests properly verify that:
- Verbose output appears in stderr
- stdout remains empty when appropriate
- Files are created correctly
- The --dry flag auto-enables verbose mode
821-869: LGTM!Good validation of version display and progress indicator behavior:
- Version output goes to stdout (not stderr)
- Progress indicator is suppressed in non-TTY environments
- Progress indicator is suppressed when verbose mode is active (per-file stats are shown instead)
871-958: LGTM!Excellent test coverage for:
- Progress indicator suppression with --dry flag
- Nested directory traversal and processing
- Avoiding infinite loops by skipping output directories nested in input directories
- Graceful handling of symbolic links (with Windows compatibility)
cli.js (9)
107-113: LGTM!This implementation properly addresses the validation requirement from previous reviews. The function correctly validates numeric inputs and provides clear error messages, preventing NaN from being passed to the minifier.
118-118: LGTM!All numeric CLI options now use the validation parser, ensuring invalid values are caught early with clear error messages.
Also applies to: 134-135
187-222: LGTM!Good implementation of progressive config loading with clear fallback chain (JSON → CJS → ESM). The error handling provides helpful diagnostic information when all formats fail, and the ESM import correctly handles both default and named exports.
249-265: LGTM!Both helper functions are well-implemented:
getActiveOptionsDisplaycorrectly identifies CLI-provided options and formats them appropriatelycalculateStatsaccurately calculates size metrics using Buffer.byteLength and handles edge cases (zero original size)
267-295: LGTM!The updated
processFilefunction correctly implements verbose mode with per-file statistics. The addition of theisVerboseparameter is backward-compatible, and the stats display format is clear and informative.
346-363: LGTM!Clean implementation of progress indicator utilities with:
- Visual progress bar using Unicode characters
- Path truncation for long filenames
- Proper ANSI escape sequences for in-place updates
- Safe handling of edge cases (zero total)
365-422: LGTM!The updated
processDirectoryfunction correctly implements:
- Verbose mode propagation through recursive calls
- Progress tracking and updates
- Symbolic link skipping
- Output directory avoidance to prevent infinite loops
424-475: LGTM!The
writeMinifyfunction correctly integrates verbose and dry-run modes with appropriate stats display, and the options display helps users understand what configuration is active.
490-549: LGTM!Excellent implementation of the directory processing logic with:
- Automatic verbose mode when using --dry flag
- Progress indicator only in appropriate contexts (TTY, non-verbose)
- Proper file counting before processing
- Clean progress cleanup
- Comprehensive total statistics display
Updated changelog to reflect refined flag descriptions, improved CLI documentation, and expanded test coverage. This ensures better clarity about recent enhancements and internal changes for users. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Simplified progress updates to show file count and percentage, refreshing only every 10% or on the last file. Enhanced user feedback with clear completion messages and optimized TTY behavior for better terminal display. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Removed unused parameters and redundant condition checks from the `updateProgress` function, streamlining its implementation. […] (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Refined the changelog to reflect the updated description of the progress indicator for directory processing, removing mention of the 10% update frequency. This ensures users are properly informed about the latest behavior of this feature. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
CHANGELOG.md (1)
19-19: Rephrase "explicitly provided disabled options" for clarity (flagged previously).This phrasing remains unclear. Please clarify that when users pass flags with the
--no-prefix (e.g.,--no-html5), verbose mode displays them without the leading dashes (asno-html5). The current wording "explicitly provided disabled options" is ambiguous and doesn't clearly convey this behavior.Suggest rephrasing to: "Verbose mode displays options explicitly disabled by the user using the
--no-prefix (e.g.,--no-html5shown asno-html5)" or similar for clarity. Also verify that the implementation in the CLI code actually strips the leading dashes before rendering verbose output.cli.js (1)
315-344: Silent error handling remains unaddressed.As noted in previous reviews, the function silently ignores errors from
readdir(line 318),realpath(line 325), andlstat(line 331) operations. While this issue was accepted, it hasn't been implemented yet. Users won't know why certain files weren't counted, which could lead to misleading progress indicators.Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)cli.js(14 hunks)
🔇 Additional comments (4)
CHANGELOG.md (2)
11-13: Changelog entries now concise and well-organized.The version 3.2.0 Added entries have been successfully simplified compared to earlier iterations. Each feature (verbose flag, progress indicator, ESM config support) is now clear and readable with appropriately scoped details.
44-46: Test coverage breakdown now clear and well-structured.The three test coverage categories have been successfully split into separate bullets with consistent, parallel phrasing. Each entry is now easy to scan and understand at a glance. This directly addresses the prior feedback on readability.
cli.js (2)
107-113: LGTM! Addresses previous validation concerns.The
parseValidIntfunction correctly validates numeric input and provides clear error messages, addressing the previous review feedback about NaN handling.
187-204: LGTM! Robust config loading with comprehensive fallbacks.The enhanced config loading correctly attempts JSON → CJS → ESM with proper error handling and descriptive messages. The use of
pathToFileURLfor ESM imports is appropriate.
Enhanced progress system to support indeterminate states when total file count is unknown. Updated completion messages to accurately reflect the number of files processed, ensuring better user feedback. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cli.js(14 hunks)
⏰ 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: Node current (windows-latest)
🔇 Additional comments (9)
cli.js (9)
107-113: LGTM! Proper input validation added.The numeric input validation correctly prevents NaN values from being passed to the minifier, with clear error messages including the option name.
249-256: Consider documenting the CLI-only filter behavior.The function currently only displays options set via CLI flags (not from config files). This is likely intentional to show what the user explicitly provided, but worth confirming this aligns with expectations for verbose mode.
258-265: LGTM! Accurate byte counting with proper edge case handling.The function correctly uses
Buffer.byteLengthfor multi-byte character support and handles the division-by-zero case when calculating percentages.
267-295: LGTM! Clean separation of concerns with dry-run and verbose modes.The function properly computes statistics, conditionally displays them, and returns data for aggregation while maintaining existing error handling.
346-361: LGTM! Robust progress indicator with determinate and indeterminate modes.The implementation correctly handles both known and unknown totals, uses appropriate ANSI codes for terminal control, and prevents display anomalies with the ratio clamping.
363-420: LGTM! Proper parameter threading and progress updates.The function correctly propagates verbose and progress parameters through recursive calls and updates progress after each file is processed.
187-204: LGTM! Comprehensive config loading with clear error messages.The three-tier fallback (JSON → CJS → ESM) properly handles all common module formats, and the error message helpfully shows all failure reasons.
422-473: LGTM! Consistent verbose and dry-run behavior for single-file processing.The implementation maintains consistency with the directory processing path, properly showing configuration and statistics as appropriate.
517-534: Clever solution: asynchronous counting minimizes double-traversal impact.The implementation starts processing immediately with an indeterminate progress indicator, then switches to determinate when the background count completes. This addresses the performance concern from previous reviews while maintaining good UX.
Enhanced the comment to clarify asynchronous behavior of `countFiles` and its safe mutation of shared state in JavaScript. This ensures developers understand the transition from indeterminate to determinate progress without race conditions. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Replaced curly quotation marks with backticks for consistent formatting of inline code examples. This ensures better readability and aligns with common documentation practices. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cli.js (1)
315-344: Don’t silently swallow filesystem errors during counting.
readdir/realpath/lstatfailures are ignored, leading to misleading totals and progress. Log a warning and continue so users know files were skipped.- const files = await fs.promises.readdir(dir).catch(() => []); + const files = await fs.promises.readdir(dir).catch((err) => { + if (err?.code !== 'ENOENT') { + console.error(`Warning: Cannot read directory ${dir}: ${err.message}`); + } + return []; + }); @@ - const lst = await fs.promises.lstat(filePath).catch(() => null); + const lst = await fs.promises.lstat(filePath).catch((err) => { + console.error(`Warning: Cannot stat ${filePath}: ${err.message}`); + return null; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(7 hunks)cli.js(14 hunks)
🔇 Additional comments (2)
cli.js (2)
107-114: Validated numeric parsing looks good.
parseValidIntwith explicit error messages fixes prior NaN hazards for numeric flags (e.g.,--max-line-length=abc). Good improvement.Also applies to: 118-118, 134-135
346-357: Progress indicator and shared-state notes: LGTM.
- Background counting with indeterminate → determinate transition is documented and works well on TTY only.
- Progress updates after each file and clear-on-complete are correct.
Please confirm tests cover non-TTY (CI) output to avoid spurious control codes in logs.
Also applies to: 359-361, 363-420, 517-538, 540-547, 548-557
Extracted config loading and normalization into dedicated functions for better modularity and maintainability. Improved error handling and documentation to ensure more robust and clear behavior in configuration management. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Updated CLI error message tests to use backticks and improve clarity in option names and descriptions. These changes ensure consistency and readability across error outputs. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Renamed the "CLI options" heading to "CLI" for brevity and clarity in the README. This aligns with simplified documentation practices and improves readability. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Adjusted the async option parser to a synchronous path capture with explicit post-parse loading. This ensures the configuration is fully loaded and normalized before any minification operations occur. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli.js (1)
80-91: Broaden JSON parse error signal for array-like inputs.Currently only values starting with “{” trigger an error message on JSON parse failure. Arrays like
[...]with bad syntax fall through as strings.Apply this diff:
- } catch { - if (/^{/.test(value)) { + } catch { + if (/^\s*[\{\[]/.test(value)) { fatal('Could not parse JSON value `' + value + '`'); } return value; }
♻️ Duplicate comments (5)
CHANGELOG.md (1)
18-20: Wording clarification was addressed.“Explicitly provided disabled options” is now unambiguous with the
--no-html5→no-html5example. Nothing further.cli.js (3)
186-246: ESM config race fixed; normalization looks solid.Switch to
parseAsyncand post-parseloadConfigFromPath()resolves the earlier TOCTOU on ESM config loading; the JSON→CJS→ESM chain andnormalizeConfig()are correct. Nice interoperability handling for__esModule.
282-289: Label suggests all options; output lists CLI-sourced only.Header “Options:” prints only CLI‑sourced flags. Either rename to “CLI options:” or include config-sourced ones when
--config-fileis used. UX nit; no behavior change.
348-377: Swallowed FS errors hide why files are skipped.
readdir/realpath/lstatfailures return[]/nullwithout notice, which can miscount progress silently. Consider logging non‑ENOENT warnings (operation + path + message).README.md (1)
228-241: Clarify verbose header matches CLI behavior.Examples show “Options: …” which are CLI‑sourced flags only. Consider “CLI options:” (or a note that config‑derived values may also change behavior).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)README.md(7 hunks)cli.js(6 hunks)tests/cli.spec.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli.js (1)
src/htmlminifier.js (6)
result(782-782)result(1457-1457)option(715-715)options(692-712)minify(1454-1460)minify(1454-1460)
🔇 Additional comments (3)
CHANGELOG.md (1)
11-13: Readable, scoped bullets for verbose/progress/ESM.Changelog entries are clear and match implemented behavior (TTY‑only progress, suppressed by --verbose/--dry; ESM via dynamic import). Looks good.
tests/cli.spec.js (2)
357-383: Add CJS and ESM config loading tests to cover JSON→CJS→ESM chain.Both module types load and normalize
fileExtcorrectly. The infrastructure exists:execCliWithStderr()is widely used, config loading already supports CJSrequire()and ESMimport(), andfileExtnormalization handles both string and array formats.test('should process files with extensions from CJS config', () => { fs.mkdirSync(path.resolve(fixturesDir, 'tmp'), { recursive: true }); fs.writeFileSync(path.resolve(fixturesDir, 'tmp/test-config.cjs'), 'module.exports = { fileExt: ["htm"], collapseWhitespace: true };'); const result = execCliWithStderr([ '--config-file=./tmp/test-config.cjs', '--input-dir=./', '--output-dir=./tmp/config-cjs' ]); assert.strictEqual(result.exitCode, 0); assert.strictEqual(existsFixture('tmp/config-cjs/extension.htm'), true); assert.strictEqual(existsFixture('tmp/config-cjs/extension.html'), false); fs.unlinkSync(path.resolve(fixturesDir, 'tmp/test-config.cjs')); }); test('should process files with extensions from ESM config', () => { fs.mkdirSync(path.resolve(fixturesDir, 'tmp'), { recursive: true }); fs.writeFileSync(path.resolve(fixturesDir, 'tmp/test-config.mjs'), 'export default { fileExt: "html,htm", collapseWhitespace: true };'); const result = execCliWithStderr([ '--config-file=./tmp/test-config.mjs', '--input-dir=./', '--output-dir=./tmp/config-esm' ]); assert.strictEqual(result.exitCode, 0); assert.strictEqual(existsFixture('tmp/config-esm/extension.html'), true); assert.strictEqual(existsFixture('tmp/config-esm/extension.htm'), true); fs.unlinkSync(path.resolve(fixturesDir, 'tmp/test-config.mjs')); });
691-716: Proposed test is valid and correctly captures verbose output behavior for disabled flags.The review suggestion is correct. The codebase's
getActiveOptionsDisplayfunction (cli.js line 285) specifically formats disabled boolean options asno-${k}, so using--no-html5will indeed display asno-html5in stderr. The proposed test appropriately complements the existing verbose test—which validates enabled options like--collapse-whitespace—by verifying that explicitly disabled flags (--no-*) are properly displayed. The test structure follows existing patterns and uses valid CLI options and helper functions.
Enhanced numeric option validation to reject non-numeric values, values with trailing characters, and negatives with precise error messages. Updated related tests to cover edge cases and improved robustness in CLI error handling. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)cli.js(6 hunks)tests/cli.spec.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli.js (2)
src/htmlminifier.js (6)
result(782-782)result(1457-1457)option(715-715)options(692-712)minify(1454-1460)minify(1454-1460)tests/minifier.spec.js (5)
result(4103-4103)result(4127-4127)result(4163-4163)options(3086-3086)options(3258-3263)
⏰ 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: Node current (windows-latest)
🔇 Additional comments (13)
tests/cli.spec.js (5)
98-107: LGTM: Improved error message formatting.The backticks around option names enhance readability and align with the CLI documentation improvements in this PR.
109-179: Excellent numeric validation test coverage.These tests comprehensively validate that the CLI rejects:
- Pure non-numeric strings (abc, xyz, invalid)
- Trailing characters after digits (12abc, 99KB, 100x)
- Negative numbers (-50, -100)
This addresses the parseInt pitfall identified in past review comments where
parseInt('12abc', 10)would silently accept the numeric prefix.
627-635: LGTM: EPIPE handling test validates Unix pipeline behavior.This test ensures the CLI gracefully handles broken pipes when downstream processes like
headclose early, preventing noisy error messages and ensuring clean exit codes.
736-872: LGTM: Comprehensive verbose mode test coverage.These tests thoroughly validate:
- Per-file statistics in verbose mode (single file, directory, STDIN)
- Options display showing active configuration
- Automatic verbose activation with
--dryflag- Proper stderr/stdout separation
- Version flag output format
The test coverage aligns well with the verbose implementation and ensures the feature works across all I/O scenarios.
874-1003: LGTM: Excellent progress and edge case test coverage.These tests thoroughly validate:
- Progress indicator correctly disabled in non-TTY environments (CI/piped)
- Progress suppressed when
--verboseor--dryflags are active- Nested directory traversal with correct file structure preservation
- Skip logic for output directory nested in input directory (prevents reprocessing)
- Symbolic link skipping (avoids infinite loops and duplicates)
The edge case coverage demonstrates careful attention to potential pitfalls in directory processing.
CHANGELOG.md (2)
23-24: LGTM: Detailed and accurate fix descriptions.Both entries clearly explain the problem and solution:
- Numeric validation fix addresses the
parseIntpitfall with concrete examples- Race condition fix provides technical detail about the async loading issue
The level of detail helps users understand the impact and developers understand the implementation.
28-30: LGTM: Appropriate internal change documentation.The internal changes are clearly documented with sufficient detail for maintainers, including:
- Code refactoring (processFile consistency)
- Test coverage additions (edge cases)
The level of detail is appropriate for an Internal section.
cli.js (6)
107-115: LGTM: Robust numeric validation implementation.The
parseValidIntfunction properly addresses theparseIntpitfall by using a strict regex (/^\d+$/) that:
- Rejects trailing characters (e.g., "12abc")
- Rejects negative numbers (e.g., "-50")
- Rejects pure non-numeric strings (e.g., "abc")
The implementation aligns with the test coverage in tests/cli.spec.js (lines 109-179) and provides clear error messages.
188-247: LGTM: Config loading refactor fixes race condition.This refactor properly addresses the async config loading race condition identified in past review comments by:
- Replacing the async option parser with a plain synchronous option (line 250)
- Loading config explicitly after
parseAsynccompletes (lines 264-267)- Separating concerns:
loadConfigFromPathhandles file loading,normalizeConfighandles transformation- Supporting JSON → CJS → ESM fallback with clear error messages
- Handling ESM interop correctly (line 206)
The synchronous-option + post-parse-load pattern ensures config is fully loaded before minification operations.
381-396: LGTM: Clean progress indicator implementation.The
updateProgressandclearProgressfunctions provide a clean progress UI using ANSI escape codes. The implementation:
- Shows determinate progress (percentage) when total is known
- Shows indeterminate progress when total is not yet available
- Cleanly clears the line when complete
The progress is only shown when TTY is detected (line 553), preventing noise in piped/CI environments.
398-455: LGTM: processDirectory properly handles verbose, progress, and skip logic.The refactored function correctly:
- Threads verbose and progress parameters through recursive calls
- Skips output directory when nested in input (prevents reprocessing)
- Skips symbolic links (avoids infinite loops)
- Updates progress after each file processed
- Returns stats for all files (used in verbose summary)
The implementation aligns with the comprehensive test coverage in tests/cli.spec.js (lines 940-1003).
556-573: LGTM: Well-documented shared-state mutation pattern.The code properly documents the async progress counting pattern with a clear comment (lines 560-564) explaining:
countFilesmutatesprogress.totalasynchronouslyupdateProgressreads the value during processing- Safety is guaranteed by JavaScript's single-threaded execution
- Progress transitions smoothly from indeterminate to determinate
This addresses the past review comment requesting explicit documentation of the shared-state pattern.
255-603: LGTM: Main CLI flow properly implements verbose, progress, and config features.The refactored main flow correctly:
- Uses
parseAsyncfor proper async handling (line 259)- Loads and normalizes config after parsing (lines 264-267)
- Auto-enables verbose mode with
--dryflag (line 525)- Shows progress only in TTY and non-verbose/non-dry mode (line 553)
- Displays completion messages and aggregate stats in verbose mode (lines 583-592)
- Handles all I/O modes: directory, files, STDIN (lines 516-602)
The implementation aligns with PR objectives and extensive test coverage.
Enhanced JSON parsing to properly handle malformed array-like inputs and JSON with leading whitespace, providing clear error messages. Updated CLI output formatting to consistently use "CLI options" for better clarity and context. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)cli.js(6 hunks)tests/cli.spec.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/cli.spec.js (1)
cli.js (1)
result(204-204)
cli.js (2)
src/htmlminifier.js (6)
result(782-782)result(1457-1457)option(715-715)options(692-712)minify(1454-1460)minify(1454-1460)tests/minifier.spec.js (5)
result(4103-4103)result(4127-4127)result(4163-4163)options(3086-3086)options(3258-3263)
🔇 Additional comments (21)
cli.js (15)
30-30: LGTM! ESM config support requires pathToFileURL.The
pathToFileURLimport is necessary for converting absolute filesystem paths tofile://URLs when dynamically importing ESM configs.
84-86: LGTM! Improved JSON error detection.The updated regex correctly detects JSON-like values (objects/arrays) with leading whitespace, ensuring malformed inputs like
{bad: json}are caught and reported with clear error messages.
107-115: LGTM! Strict integer validation addresses previous concerns.The implementation correctly rejects invalid inputs including trailing characters (e.g.,
12abc), negative numbers, and pure non-numeric strings. Using/^\d+$/withNumber()ensures strict validation withoutparseIntpitfalls.
188-217: LGTM! Robust config loading with proper error handling.The function correctly attempts JSON → CJS → ESM loading with comprehensive error reporting. The ESM interop handling (lines 205-206) correctly handles cases where
require()loads a transpiled ESM module with__esModuleflag.
219-247: LGTM! Config normalization ensures consistent option handling.The function properly normalizes config values by applying parsers and converting
fileExtarrays to comma-separated strings. Using spread operator prevents mutation of the original config object.
255-267: LGTM! Post-parse config loading eliminates race condition.Using
parseAsyncand loading config after parsing completes (lines 264-267) ensures the config is fully loaded and normalized before any minification operations. This addresses the critical race condition identified in previous reviews.
284-291: LGTM! Clear labeling of CLI-sourced options.The label "CLI options:" appropriately clarifies that only command-line flags are shown (not config-file options). The boolean option formatting (e.g.,
no-html5) correctly represents explicitly disabled flags.
293-300: LGTM! Stats calculation is accurate and safe.The function correctly calculates byte sizes, handles cases where minified output is larger than input, and safely computes percentages with a zero-division guard.
302-330: LGTM! Enhanced processFile with verbose and dry-run support.The updated signature and implementation correctly:
- Reports per-file statistics in verbose/dry-run modes
- Skips writing in dry-run mode (lines 321-323)
- Returns consistent stats objects for aggregation
- Handles errors appropriately
332-348: LGTM! Robust file extension parsing.The implementation correctly:
- Normalizes extensions (trim, lowercase, remove leading dots)
- Deduplicates using Set
- Defaults to processing all files when no extensions specified
350-379: LGTM! File counting for progress tracking with appropriate error handling.The function correctly:
- Recursively counts files matching specified extensions
- Skips output directory to avoid counting output files
- Skips symbolic links to avoid double-counting or cycles
- Silently handles errors (acceptable for optional progress feature)
381-396: LGTM! Clean progress indicator implementation.The progress functions correctly use ANSI escape sequences for TTY updates and gracefully handle both determinate (with total) and indeterminate (counting in progress) modes.
398-455: LGTM! Enhanced directory processing with verbose and progress support.The function correctly:
- Threads verbose, progress, and skipRootAbs parameters through recursive calls
- Skips output directory and symlinks
- Updates progress after each file
- Aggregates stats for summary reporting
- Handles both dry-run and normal modes
457-508: LGTM! Enhanced writeMinify with comprehensive verbose and dry-run support.The function correctly:
- Displays active options in verbose/dry modes
- Shows statistics in verbose mode
- Skips writing in dry-run mode
- Uses stream-based writing for efficiency
- Handles directory creation and errors appropriately
510-603: LGTM! Main flow with well-documented progress tracking.The implementation correctly:
- Resolves CLI vs. config file extension precedence
- Auto-enables verbose mode with
--dry- Skips output directory during traversal
- Documents the safe shared-state mutation pattern (lines 560-565) as requested in previous reviews
- Shows completion messages and total statistics
- Handles all three input modes (directory, file arguments, STDIN)
tests/cli.spec.js (6)
98-107: LGTM! Error message assertions aligned with improved CLI output.The updated assertions correctly expect backticks around option names in error messages, matching the enhanced error reporting in the CLI implementation.
109-197: LGTM! Comprehensive validation test coverage addresses previous gaps.The test suite now thoroughly covers:
- Invalid non-numeric inputs
- Trailing characters (e.g.,
12abc,99KB) — addressing previous review concerns- Negative numbers (correctly rejected)
- Malformed JSON inputs with leading whitespace
All assertions correctly match the strict validation logic implemented in
parseValidInt.
645-653: LGTM! EPIPE handling test ensures graceful pipe termination.The test correctly verifies that the CLI exits cleanly (code 0) without noisy errors when the output pipe is broken, simulating real-world scenarios like piping to
head.
754-882: LGTM! Thorough verbose mode test coverage.The test suite comprehensively validates verbose behavior across:
- Single file, directory, and STDIN inputs
- Proper stderr output (options, stats, markers)
- Auto-enable behavior with
--dry- Negative case (no verbose output without flag)
- File creation verification
All assertions correctly verify the expected output format.
884-956: LGTM! Version display and progress indicator tests validate TTY-aware behavior.The tests correctly verify:
- Version flag outputs valid semver format
- Progress indicator is suppressed in non-TTY environments
- Progress is disabled when
--verboseor--dryflags are used- Per-file stats are shown instead of progress bar in verbose/dry modes
958-1021: LGTM! Directory traversal tests cover important edge cases.The tests correctly validate:
- Recursive processing of nested subdirectories
- Skipping output directory when nested in input directory (prevents infinite loops)
- Skipping symbolic links (with appropriate Windows fallback handling)
These tests ensure robust directory traversal behavior.
Updated the changelog for better clarity on the progress indicator, adjusting punctuation to improve readability. This ensures the changelog aligns with high editorial standards. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Resolves #49
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores