Ready to drop “beta” (e.g., improve config validation and symlink handling)#4
Conversation
Added explicit error messages for invalid config values (e.g., `links.timeout`, `links.warnOnPermanentRedirects`). Updated directory traversal logic to follow symlinked files while skipping symlinked directories to prevent cycles. Adjusted quiet mode message handling to correctly display report suggestions. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR releases version 1.3.2 with centralized configuration validation, symlink-aware directory traversal that follows in-root symlinked HTML files while skipping symlinked directories, and refined quiet-mode CLI hints. Documentation and package version are updated accordingly. Changesv1.3.2 Release
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@bin/hihtml.js`:
- Line 147: The guard that decides whether to show the quiet-mode hint is
inverted: in the code around the conditional using opts.quiet and sections it
currently returns when (!opts.quiet || sections.length === 0); change the logic
so the function returns early only when not in quiet mode or when there are
existing sections (i.e., flip sections.length === 0 to sections.length > 0) so
the hint is shown when quiet is enabled and no sections exist; update the
conditional that references opts.quiet and sections accordingly (the surrounding
block that emits the hint).
In `@src/lib/files.js`:
- Line 84: The bare catch around the stat/lstat call swallows all errors; change
it to catch the error object (e.g., catch (err)) and only ignore expected
"broken symlink" errno values (at minimum 'ENOENT' and 'ELOOP') while rethrowing
any other errors so real permission/IO problems surface. Locate the try/catch
around the stat() call (the block that currently reads catch { /* broken
symlink—skip */ }) and replace it with logic that checks err.code and
continues/returns for the known symlink-related codes but throws err for
anything else.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 27b8b532-29ec-4b7e-9f69-4486ed42ac73
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
CHANGELOG.mdREADME.mdbin/hihtml.jsbin/hihtml.test.jspackage.jsonsrc/lib/config.jssrc/lib/files.js
Improved error handling to exclude specific error codes (`ENOENT`, `ELOOP`, `EACCES`, `EPERM`) while preserving existing behavior for broken symlinks. Ensures unexpected errors are rethrown to avoid masking critical issues. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Pull request overview
This PR prepares hihtml for a non-beta release by improving robustness and clarity around configuration handling, file traversal behavior, and user-facing messaging.
Changes:
- Follow symlinked HTML files during directory traversal while continuing to skip symlinked directories to avoid cycles.
- Add strict config shape/type validation with clearer error messages, plus new regression tests for invalid config values.
- Update CLI quiet-mode hint messaging and refresh docs/versioning for the 1.3.2 release.
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/lib/files.js |
Updates recursive walk logic to include symlinked HTML files (but skip symlinked dirs). |
src/lib/config.js |
Adds runtime config validation to fail fast with clearer errors on malformed config. |
bin/hihtml.js |
Adjusts --quiet hint messaging to avoid suggesting -r when report output is already enabled. |
bin/hihtml.test.js |
Adds tests asserting config validation errors for invalid types/values. |
README.md |
Removes “Beta” branding and documents symlink traversal + informational warning exit code behavior. |
CHANGELOG.md |
Adds release notes for 1.3.2 dated 2026-05-18. |
package.json |
Bumps version from 1.3.1-beta to 1.3.2. |
package-lock.json |
Updates lockfile version metadata to match 1.3.2. |
Comments suppressed due to low confidence (2)
src/lib/files.js:88
- This change adds new symlink traversal behavior but there’s no corresponding test coverage. Since the repo already tests
collect()inbin/hihtml.test.js, please add tests that (1) a symlinked.htmlfile is included and (2) a symlinked directory is not traversed (to prevent cycles), with platform guards where necessary.
if (entry.isSymbolicLink()) {
try {
const resolved = await fs.promises.stat(fullPath);
if (resolved.isFile()) {
const ext = path.extname(entry.name).slice(1).toLowerCase();
if (extensions.has(ext)) results.push(fullPath);
}
// Symlinked directories are skipped to prevent cycles
} catch (err) {
const code = /** @type {NodeJS.ErrnoException} */ (err).code;
if (code !== 'ENOENT' && code !== 'ELOOP' && code !== 'EACCES' && code !== 'EPERM') throw err;
}
continue;
src/lib/files.js:80
resolvedhere is afs.Statsobject (result ofstat()), not a resolved path. Renaming it to something likestatswould make the intent clearer and avoid confusion with theresolvedpath variable used elsewhere in this module.
const resolved = await fs.promises.stat(fullPath);
if (resolved.isFile()) {
const ext = path.extname(entry.name).slice(1).toLowerCase();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Updated traversal logic to follow symlinked files only when their targets resolve within the scanned root, skipping symlinks pointing outside the root or to directories. Added corresponding unit tests to ensure correct behavior and prevent regressions. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Updated symlink handling to ensure `realpath` calculations respect the scanned root directory. Improved logic to handle relative paths correctly and skip invalid symlinks pointing outside the root, preventing unintended traversal. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores