Merged
Conversation
Streamlined regex creation for elements and attributes to enhance performance. Added excluded directories and updated file traversal to handle more extensions and symbolic links efficiently. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Added new test cases for JSX, TSX, and edge cases for attribute positions. Streamlined test execution by introducing a utility function for running the script and simplifying assertions. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
WalkthroughThe PR rewrites the obsohtml CLI to synchronous processing with precompiled regexes for obsolete elements/attributes, adds EXCLUDED_DIRS and symlink skipping in directory traversal, expands file extensions to include .jsx/.tsx, improves error/verbose handling, updates tests, README, and makes the script exit with code 1 when obsolete HTML is found. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (bin/obsohtml.js)"
participant FS as "Filesystem (walkDirectory / file)"
participant Finder as "findObsolete (precompiled regexes)"
participant Logger as "Logger / stdout/stderr"
participant Proc as "Process (exit)"
CLI->>FS: accept path (file or dir)
alt path is directory
CLI->>FS: walkDirectory (sync), skipping EXCLUDED_DIRS & symlinks
FS->>CLI: file paths
loop per file
CLI->>Finder: run findObsolete(file)
Finder->>Logger: report matches (stdout)
Finder-->>CLI: set foundObsolete true if matches
end
else path is file
CLI->>Finder: run findObsolete(file)
Finder->>Logger: report matches (stdout)
Finder-->>CLI: set foundObsolete true if matches
end
CLI->>Logger: verbose errors/info as needed
alt foundObsolete true
CLI->>Proc: exit(1)
else
CLI->>Proc: exit(0)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 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 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 |
Clarified wording on supported paths and added details about return codes for CI compatibility. These updates improve comprehensibility and highlight key features for users. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
29-29: 🧹 Nitpick | 🔵 TrivialConsider bumping the version for the breaking exit-code change.
The new
process.exit(1)path is a semantic breaking change: any CI/CD pipeline that previously relied on the tool always exiting0will now fail unexpectedly. Semver convention suggests at minimum a minor bump, and arguably a major one (e.g.,2.0.0), to communicate this to consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 29, Update the package version field to reflect the breaking change introduced by the new exit behavior: bump the "version" value in package.json to an appropriate new semver (at minimum a minor bump, e.g., 1.10.0, or a major bump such as 2.0.0) so consumers are notified of the semantic change caused by the added process.exit(1) path; modify the "version" string accordingly and ensure any release or changelog steps are aligned with that new version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/obsohtml.js`:
- Line 68: The constant MAX_PATH_LENGTH is using 255 (NAME_MAX) but is applied
as a full-path limit; change the guard in bin/obsohtml.js to use a realistic
full-path limit (e.g., PATH_MAX ≈ 4096) or remove the guard entirely so the fs
calls can surface ENAMETOOLONG themselves; update any checks that reference
MAX_PATH_LENGTH (the constant and any conditional that skips files based on it)
to use the new PATH_MAX value or to remove that conditional check.
- Around line 44-64: findObsolete currently calls fs.readFileSync(filePath)
without handling EPERM/EACCES, so permission errors bubble up and abort the
recursive walkDirectory; modify findObsolete to catch and silently handle
read-permission errors (EPERM/EACCES) around the readFileSync call (similar to
how walkDirectory handles readdirSync), ensuring it sets/uses foundObsolete as
before and continues the scan when encountering unreadable files; reference the
findObsolete function, readFileSync call, and elementRegexes/attributeRegexes to
locate where to wrap the read and short-circuit on permission errors.
- Around line 123-137: The module-level flag foundObsolete causes stale state
across multiple main() invocations; make the flag local to main by declaring let
foundObsolete = false at the top of main(), then change findObsolete(...) and
walkDirectory(...) to report their result instead of mutating module state —
either return a boolean (or Promise<boolean>) or accept a callback/setter so
they can set the local foundObsolete; finally use the local foundObsolete to
decide process.exit(1). Ensure all call sites (the uses of findObsolete and
walkDirectory) are updated to consume their return values or pass the setter.
- Around line 103-109: The test suite lacks a fixture asserting that files with
the .ts extension are scanned: add a new fixture and test case mirroring the
existing .jsx and .tsx tests to exercise the code path that checks
fullPath.endsWith('.ts') (or update the existing test harness that validates
extensions). Specifically, create a .ts file fixture, add assertions in the same
test file that verifies the scanner picks up .ts files (similar to the .jsx/.tsx
tests), and run the relevant test to ensure fullPath.endsWith('.ts') remains
covered.
In `@bin/obsohtml.test.js`:
- Around line 55-86: Several tests (e.g., the "Detect obsolete elements",
"Detect obsolete attributes", "Detect obsolete minimized attributes", and
"Detect obsolete elements in Twig file" tests) currently call run(['-f',
tempDir]) which makes them pass as long as any fixture in tempDir contains the
expected token; change these to target the specific fixture that should produce
the expectation (for example replace run(['-f', tempDir]) with run(['-f',
path.join(tempDir, '<specificFixtureName>')]) or run(['--folder',
path.relative(process.cwd(), path.join(tempDir, '<specificFixtureName>'))]) so
each test (like the assertions in tests named Detect obsolete
elements/attributes/minimized attributes and Detect obsolete elements in Twig
file) only runs against its intended file (or if you choose not to change the
path, add a one-line comment above each test referencing which fixture provides
the match, e.g., // expected from tempFile or tempJsxFile).
---
Outside diff comments:
In `@package.json`:
- Line 29: Update the package version field to reflect the breaking change
introduced by the new exit behavior: bump the "version" value in package.json to
an appropriate new semver (at minimum a minor bump, e.g., 1.10.0, or a major
bump such as 2.0.0) so consumers are notified of the semantic change caused by
the added process.exit(1) path; modify the "version" string accordingly and
ensure any release or changelog steps are aligned with that new version.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
bin/obsohtml.jsbin/obsohtml.test.jspackage.json
Comment on lines
+44
to
64
| function findObsolete(filePath) { | ||
| const content = fs.readFileSync(filePath, 'utf8'); | ||
|
|
||
| // Check for obsolete elements | ||
| obsoleteElements.forEach(element => { | ||
| const elementRegex = new RegExp(`<\\s*${element}\\b`, 'i'); | ||
| if (elementRegex.test(content)) { | ||
| for (const { element, regex } of elementRegexes) { | ||
| if (regex.test(content)) { | ||
| foundObsolete = true; | ||
| const message = styleText('blue', `Found obsolete element ${styleText('bold', `'${element}'`)} in ${filePath}`); | ||
| console.log(message); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| // Check for obsolete attributes | ||
| obsoleteAttributes.forEach(attribute => { | ||
| const attributeRegex = new RegExp(`<[^>]*\\s${attribute}\\b(\\s*=\\s*(?:"[^"]*"|'[^']*'|[^"'\\s>]+))?\\s*(?=/?>)`, 'i'); | ||
| if (attributeRegex.test(content)) { | ||
| for (const { attribute, regex } of attributeRegexes) { | ||
| if (regex.test(content)) { | ||
| foundObsolete = true; | ||
| const message = styleText('green', `Found obsolete attribute ${styleText('bold', `'${attribute}'`)} in ${filePath}`); | ||
| console.log(message); | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unreadable file (EPERM/EACCES) from readFileSync crashes the entire traversal.
findObsolete is called inside walkDirectory's inner try block (line 110). That block's catch (lines 112–118) only silences ENOENT — any EPERM/EACCES thrown by readFileSync is re-thrown, which unwinds all recursive walkDirectory frames and terminates the scan. By contrast, readdirSync permission errors are gracefully handled at lines 74–76.
🛡️ Proposed fix — handle read-permission errors in the inner catch
} catch (err) {
if (err.code === 'ENOENT') {
if (verbose) console.warn(`Skipping non-existent file or directory: ${fullPath}`);
+ } else if (err.code === 'EPERM' || err.code === 'EACCES') {
+ if (verbose) console.warn(`Skipping file or directory due to permissions: ${fullPath}`);
} else {
throw err;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/obsohtml.js` around lines 44 - 64, findObsolete currently calls
fs.readFileSync(filePath) without handling EPERM/EACCES, so permission errors
bubble up and abort the recursive walkDirectory; modify findObsolete to catch
and silently handle read-permission errors (EPERM/EACCES) around the
readFileSync call (similar to how walkDirectory handles readdirSync), ensuring
it sets/uses foundObsolete as before and continues the scan when encountering
unreadable files; reference the findObsolete function, readFileSync call, and
elementRegexes/attributeRegexes to locate where to wrap the read and
short-circuit on permission errors.
| // Function to walk through the project directory, excluding common build/VCS directories | ||
| function walkDirectory(directory, verbose) { | ||
| const MAX_PATH_LENGTH = 255; // Adjust this value based on your OS limits | ||
| const MAX_PATH_LENGTH = 255; |
There was a problem hiding this comment.
MAX_PATH_LENGTH = 255 is the per-component limit (NAME_MAX), not the full-path limit.
In Unix environments, PATH_MAX (4096 bytes) and NAME_MAX (255 bytes) are distinct limits. 255 is NAME_MAX — the limit on path components; a full path can contain many components and is therefore much longer. Using 255 as the full-path guard will silently skip legitimate files in any reasonably nested project (e.g., a monorepo with paths like /home/user/projects/client/frontend/src/components/feature/subfeature/index.html).
🛠️ Proposed fix — use a realistic full-path limit or remove the guard
- const MAX_PATH_LENGTH = 255;
+ // PATH_MAX on Linux is 4096; on macOS 1024. Use the more conservative value.
+ const MAX_PATH_LENGTH = 1024;Alternatively, remove the guard entirely and let fs syscalls raise ENAMETOOLONG natively — the existing catch will then surface it as an unhandled error you can handle explicitly.
📝 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.
Suggested change
| const MAX_PATH_LENGTH = 255; | |
| // PATH_MAX on Linux is 4096; on macOS 1024. Use the more conservative value. | |
| const MAX_PATH_LENGTH = 1024; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/obsohtml.js` at line 68, The constant MAX_PATH_LENGTH is using 255
(NAME_MAX) but is applied as a full-path limit; change the guard in
bin/obsohtml.js to use a realistic full-path limit (e.g., PATH_MAX ≈ 4096) or
remove the guard entirely so the fs calls can surface ENAMETOOLONG themselves;
update any checks that reference MAX_PATH_LENGTH (the constant and any
conditional that skips files based on it) to use the new PATH_MAX value or to
remove that conditional check.
Comment on lines
+103
to
+109
| } else if ( | ||
| fullPath.endsWith('.html') || fullPath.endsWith('.htm') || | ||
| fullPath.endsWith('.php') || | ||
| fullPath.endsWith('.njk') || fullPath.endsWith('.twig') || | ||
| fullPath.endsWith('.js') || fullPath.endsWith('.jsx') || | ||
| fullPath.endsWith('.ts') || fullPath.endsWith('.tsx') | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for test fixtures related to file extensions in the test file
echo "=== Searching for .ts in test file ==="
rg -n '\.ts[^x]' bin/obsohtml.test.js | head -20
echo ""
echo "=== Searching for .jsx in test file ==="
rg -n '\.jsx' bin/obsohtml.test.js | head -20
echo ""
echo "=== Searching for .tsx in test file ==="
rg -n '\.tsx' bin/obsohtml.test.js | head -20
echo ""
echo "=== Check file extension pattern around line 103-109 in obsohtml.js ==="
sed -n '100,115p' bin/obsohtml.jsRepository: j9t/obsohtml
Length of output: 984
🏁 Script executed:
#!/bin/bash
# Examine the test file structure to see how fixtures are used
echo "=== Lines 25-70 in test file (to see fixture setup and usage) ==="
sed -n '25,70p' bin/obsohtml.test.js
echo ""
echo "=== Search for test assertions using the fixtures ==="
rg -n 'tempJsxFile|tempTsxFile|tempTsFile' bin/obsohtml.test.jsRepository: j9t/obsohtml
Length of output: 2851
.ts extension is untested — add a fixture to verify it's scanned.
.jsx and .tsx each have dedicated fixtures and assertions, but .ts (added on line 108) has no corresponding test. A regression that accidentally dropped .ts from the extension list would go undetected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/obsohtml.js` around lines 103 - 109, The test suite lacks a fixture
asserting that files with the .ts extension are scanned: add a new fixture and
test case mirroring the existing .jsx and .tsx tests to exercise the code path
that checks fullPath.endsWith('.ts') (or update the existing test harness that
validates extensions). Specifically, create a .ts file fixture, add assertions
in the same test file that verifies the scanner picks up .ts files (similar to
the .jsx/.tsx tests), and run the relevant test to ensure
fullPath.endsWith('.ts') remains covered.
Comment on lines
+123
to
+137
| function main(projectDirectory = defaultProjectDirectory, verbose = false) { | ||
| let stats; | ||
| try { | ||
| stats = fs.lstatSync(projectDirectory); | ||
| } catch (err) { | ||
| if (err.code !== 'ENOENT') throw err; | ||
| } | ||
|
|
||
| if (stats?.isFile()) { | ||
| findObsolete(projectDirectory); | ||
| } else { | ||
| walkDirectory(projectDirectory, verbose); | ||
| } | ||
|
|
||
| if (foundObsolete) process.exit(1); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
foundObsolete is never reset — main() is not safe to call more than once per process.
With the current module-level let foundObsolete = false, a second call to main() in the same process (e.g., programmatic/library use) would see stale state from the previous run and could exit 1 even when the second scan found nothing. This is a non-issue for the current CLI-only usage since each invocation spawns a fresh process, but worth noting if the API is ever exposed programmatically.
♻️ Proposed refactor — scope the flag to main
-// Track whether any obsolete HTML was found
-let foundObsolete = false;
-
// Function to find obsolete elements and attributes in a file
-function findObsolete(filePath) {
+function findObsolete(filePath, state) {
const content = fs.readFileSync(filePath, 'utf8');
for (const { element, regex } of elementRegexes) {
if (regex.test(content)) {
- foundObsolete = true;
+ state.foundObsolete = true;
// ...
}
}
// ... same for attributes
}
-function walkDirectory(directory, verbose) {
+function walkDirectory(directory, verbose, state) {
// ... pass state down recursively
}
function main(projectDirectory = defaultProjectDirectory, verbose = false) {
+ const state = { foundObsolete: false };
// ... pass state into walkDirectory / findObsolete
- if (foundObsolete) process.exit(1);
+ if (state.foundObsolete) process.exit(1);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/obsohtml.js` around lines 123 - 137, The module-level flag foundObsolete
causes stale state across multiple main() invocations; make the flag local to
main by declaring let foundObsolete = false at the top of main(), then change
findObsolete(...) and walkDirectory(...) to report their result instead of
mutating module state — either return a boolean (or Promise<boolean>) or accept
a callback/setter so they can set the local foundObsolete; finally use the local
foundObsolete to decide process.exit(1). Ensure all call sites (the uses of
findObsolete and walkDirectory) are updated to consume their return values or
pass the setter.
Comment on lines
55
to
+86
| test('Detect obsolete elements', () => { | ||
| const result = spawnSync('node', [scriptPath, '-f', tempDir], { encoding: 'utf-8' }); | ||
| const output = stripVTControlCharacters(result.stdout); | ||
| assert.ok(output.includes("Found obsolete element 'center'")); | ||
| const { stdout } = run(['-f', tempDir]); | ||
| assert.ok(stdout.includes("Found obsolete element 'center'")); | ||
| }); | ||
|
|
||
| test('Detect obsolete attributes', () => { | ||
| const result = spawnSync('node', [scriptPath, '-f', tempDir], { encoding: 'utf-8' }); | ||
| const output = stripVTControlCharacters(result.stdout); | ||
| assert.ok(output.includes("Found obsolete attribute 'align'")); | ||
| const { stdout } = run(['-f', tempDir]); | ||
| assert.ok(stdout.includes("Found obsolete attribute 'align'")); | ||
| }); | ||
|
|
||
| test('Detect obsolete elements and attributes using absolute path', () => { | ||
| const absolutePath = path.resolve(tempDir); | ||
| const result = spawnSync('node', [scriptPath, '-f', absolutePath], { encoding: 'utf-8' }); | ||
| const output = stripVTControlCharacters(result.stdout); | ||
| assert.ok(output.includes("Found obsolete element 'center'")); | ||
| assert.ok(output.includes("Found obsolete attribute 'align'")); | ||
| const { stdout } = run(['-f', path.resolve(tempDir)]); | ||
| assert.ok(stdout.includes("Found obsolete element 'center'")); | ||
| assert.ok(stdout.includes("Found obsolete attribute 'align'")); | ||
| }); | ||
|
|
||
| test('Detect obsolete elements and attributes using relative path', () => { | ||
| const relativePath = path.relative(process.cwd(), tempDir); | ||
| const result = spawnSync('node', [scriptPath, '--folder', relativePath], { encoding: 'utf-8' }); | ||
| const output = stripVTControlCharacters(result.stdout); | ||
| assert.ok(output.includes("Found obsolete element 'center'")); | ||
| assert.ok(output.includes("Found obsolete attribute 'align'")); | ||
| const { stdout } = run(['--folder', path.relative(process.cwd(), tempDir)]); | ||
| assert.ok(stdout.includes("Found obsolete element 'center'")); | ||
| assert.ok(stdout.includes("Found obsolete attribute 'align'")); | ||
| }); | ||
|
|
||
| test('Detect obsolete minimized attributes', () => { | ||
| const result = spawnSync('node', [scriptPath, '-f', tempDir], { encoding: 'utf-8' }); | ||
| const output = stripVTControlCharacters(result.stdout); | ||
| assert.ok(output.includes("Found obsolete attribute 'noshade'")); | ||
| assert.ok(!output.includes("Found obsolete attribute 'nowrap'")); | ||
| const { stdout } = run(['-f', tempDir]); | ||
| assert.ok(stdout.includes("Found obsolete attribute 'noshade'")); | ||
| assert.ok(!stdout.includes("Found obsolete attribute 'nowrap'")); | ||
| }); | ||
|
|
||
| test('Detect obsolete elements in Twig file', () => { | ||
| const result = spawnSync('node', [scriptPath, '-f', tempDir], { encoding: 'utf-8' }); | ||
| const output = stripVTControlCharacters(result.stdout); | ||
| assert.ok(output.includes("Found obsolete element 'isindex'")); | ||
| const { stdout } = run(['-f', tempDir]); | ||
| assert.ok(stdout.includes("Found obsolete element 'isindex'")); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Most suite-level tests implicitly depend on multiple fixture files.
Tests like "Detect obsolete elements" and "Detect obsolete attributes" run against the full tempDir. Their assertions pass incidentally because any matching fixture triggers the output — if tempFile were removed, tempJsxFile still keeps the center assertion green. This cross-fixture coupling is invisible in the test names, making future fixture changes silently affect unrelated assertions. Consider targeting the specific fixture file per test (as the JSX/TSX tests do) or at least adding a comment documenting which fixture is the intended source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/obsohtml.test.js` around lines 55 - 86, Several tests (e.g., the "Detect
obsolete elements", "Detect obsolete attributes", "Detect obsolete minimized
attributes", and "Detect obsolete elements in Twig file" tests) currently call
run(['-f', tempDir]) which makes them pass as long as any fixture in tempDir
contains the expected token; change these to target the specific fixture that
should produce the expectation (for example replace run(['-f', tempDir]) with
run(['-f', path.join(tempDir, '<specificFixtureName>')]) or run(['--folder',
path.relative(process.cwd(), path.join(tempDir, '<specificFixtureName>'))]) so
each test (like the assertions in tests named Detect obsolete
elements/attributes/minimized attributes and Detect obsolete elements in Twig
file) only runs against its intended file (or if you choose not to change the
path, add a one-line comment above each test referencing which fixture provides
the match, e.g., // expected from tempFile or tempJsxFile).
j9t
added a commit
that referenced
this pull request
Feb 23, 2026
Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores
Documentation