Add configuration diff engine - Phase 1 of #667 #676
Conversation
WalkthroughAdds a new Configuration Diff tool: CLI and shell shims, DiffEngine, DiffFormatter, PathNormalizer, config docs/types, package export, README/docs additions, example configs, and unit tests for comparing bundler configurations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as diff-bundler-config (CLI)
participant Loader as Config Loader
participant PN as PathNormalizer
participant DE as DiffEngine
participant DF as DiffFormatter
participant Output as STDOUT/File
User->>CLI: invoke --left <L> --right <R> [options]
CLI->>Loader: load L and R (json|yaml|js|ts)
alt normalize paths
CLI->>PN: detectBasePath()/normalize(configs)
PN-->>CLI: normalized configs
end
CLI->>DE: compare(left,right,options)
DE-->>CLI: DiffResult (entries, summary, metadata)
CLI->>DF: format(result, format)
DF-->>CLI: formatted string
alt --output given
CLI->>Output: write file
else
CLI->>Output: print to STDOUT
end
CLI-->>User: exit code (0 no diffs/help, 1 diffs/errors)
opt Error
CLI-->>User: print error to STDERR and exit 1
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Files/areas to inspect closely:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Code Review: Configuration Diff Engine - Phase 1I've completed a thorough review of this PR. Overall, this is a well-implemented, well-tested, and well-documented feature. StrengthsExcellent Code Quality
Comprehensive Test Coverage
Great Documentation
User Experience
Areas for Improvement1. Potential Bugs CLI Error Handling (cli.ts:167-176): If ts-node is not available, it throws an error but still tries to require the TypeScript file on line 176. Should return early after throwing. Missing File Validation: loadConfigFile doesn't validate that the loaded config is actually an object/array before processing. 2. Performance Considerations Deep Cloning (pathNormalizer.ts:11-16): Creates a deep copy of entire config, could be memory-intensive for very large configs. Consider documenting memory considerations. Array Comparison (diffEngine.ts:115-126): Index-based comparison means reordering items shows as many changes. Should document this behavior. 3. Code Quality Suggestions Missing Trailing Newlines: Per CLAUDE.md, check bin/diff-bundler-config and lib/install/bin/diff-bundler-config Type Safety (cli.ts:133): No validation that parseInt succeeded. Invalid input becomes NaN. Error Handling (diffEngine.ts:173-178): Wildcard regex matching doesn't handle invalid patterns. 4. Testing Gaps Consider adding: CLI integration tests, large config tests, error cases (invalid formats, malformed JSON/YAML, missing files), edge cases (empty configs, deeply nested objects) 5. Documentation Enhancements
RecommendationsHigh Priority
Medium Priority
Security AssessmentOverall: Low Risk. This is a dev tool operating on local files. Dynamic require is acceptable for this use case. Consider adding a note in docs about running only trusted config files. SummaryCode Quality: 5/5 Overall: 4.5/5 Approval StatusThis PR is approved with minor suggestions. The issues identified are minor and can be addressed in follow-up PRs. The core functionality is solid, well-tested, and ready for production use. Great work on this feature! The modular design will make Phase 2-5 implementations much easier. Reviewed with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
README.md(1 hunks)bin/diff-bundler-config(1 hunks)docs/config-diff.md(1 hunks)lib/install/bin/diff-bundler-config(1 hunks)package.json(1 hunks)package/configDiffer/cli.ts(1 hunks)package/configDiffer/diffEngine.ts(1 hunks)package/configDiffer/formatter.ts(1 hunks)package/configDiffer/index.ts(1 hunks)package/configDiffer/pathNormalizer.ts(1 hunks)package/configDiffer/types.ts(1 hunks)test/configDiffer/diffEngine.test.js(1 hunks)test/configDiffer/formatter.test.js(1 hunks)test/configDiffer/pathNormalizer.test.js(1 hunks)tmp/examples/dev-config.yaml(1 hunks)tmp/examples/prod-config.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-28T02:23:21.827Z
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: This gem supports both webpack and rspack configurations
Applied to files:
README.md
🧬 Code graph analysis (7)
test/configDiffer/formatter.test.js (2)
package/configDiffer/formatter.ts (1)
DiffFormatter(4-162)package/configDiffer/index.ts (1)
DiffFormatter(3-3)
test/configDiffer/pathNormalizer.test.js (2)
package/configDiffer/index.ts (1)
PathNormalizer(4-4)package/configDiffer/pathNormalizer.ts (1)
PathNormalizer(4-143)
test/configDiffer/diffEngine.test.js (1)
package/configDiffer/diffEngine.ts (1)
DiffEngine(9-306)
package/configDiffer/pathNormalizer.ts (2)
package/configDiffer/index.ts (2)
PathNormalizer(4-4)NormalizedConfig(11-11)package/configDiffer/types.ts (1)
NormalizedConfig(44-48)
package/configDiffer/diffEngine.ts (1)
package/configDiffer/types.ts (5)
DiffOptions(34-42)DiffEntry(8-14)DiffResult(16-32)DiffOperation(1-1)DiffPath(3-6)
package/configDiffer/cli.ts (4)
package/configDiffer/types.ts (1)
DiffOptions(34-42)package/configDiffer/pathNormalizer.ts (1)
PathNormalizer(4-143)package/configDiffer/diffEngine.ts (1)
DiffEngine(9-306)package/configDiffer/formatter.ts (1)
DiffFormatter(4-162)
package/configDiffer/formatter.ts (1)
package/configDiffer/types.ts (3)
DiffResult(16-32)DiffEntry(8-14)DiffOperation(1-1)
🪛 markdownlint-cli2 (0.18.1)
docs/config-diff.md
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
15-15: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
27-27: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
34-34: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
329-329: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (23)
package.json (1)
25-25: LGTM! Export follows established conventions.The new configDiffer export is consistent with the existing export pattern (like configExporter on line 24) and properly exposes the new diff tooling functionality.
tmp/examples/dev-config.yaml (1)
1-18: LGTM! Well-structured development configuration example.This example config provides a realistic development setup that pairs nicely with the production config for demonstrating the diff tool's capabilities. The configuration choices (cheap-module-source-map, minimize: false, hot reloading) are appropriate for development.
tmp/examples/prod-config.yaml (1)
1-19: LGTM! Production configuration demonstrates key differences effectively.This production config provides excellent contrast with the development config, highlighting important differences like contenthash fingerprinting, minification, TerserPlugin, and CompressionPlugin. The pair of configs effectively demonstrates the diff tool's value.
package/configDiffer/types.ts (1)
1-48: LGTM! Comprehensive and well-structured type definitions.The type system is well-designed with:
- Clear operation types using string literal unions
- Hierarchical structure (DiffPath, DiffEntry, DiffResult)
- Flexible DiffOptions supporting various use cases
- Appropriate use of
anyfor handling arbitrary configuration objectsThe optional properties and metadata structure provide good extensibility for future enhancements.
bin/diff-bundler-config (1)
1-11: LGTM! Clean shim with proper error handling.The implementation follows best practices:
- Minimal shim delegates to TypeScript module
- Correctly passes CLI arguments via
process.argv.slice(2)- Proper promise handling with explicit exit codes
- Errors printed to stderr before exit
The pattern is consistent with the rest of the codebase and provides a clean separation between the executable entry point and the implementation logic.
README.md (1)
791-816: LGTM! Clear and helpful documentation.The new section provides:
- Practical examples showing real-world usage
- Multiple output format options (detailed, summary, JSON, YAML)
- Clear use cases that demonstrate the tool's value
- Logical placement after the "Debugging Configuration" section
- Reference to the detailed Configuration Diff Guide for comprehensive information
The documentation effectively guides users from basic usage to understanding when and why to use the tool.
lib/install/bin/diff-bundler-config (1)
1-11: LGTM! Install shim mirrors bin implementation correctly.The duplication between this file and
bin/diff-bundler-configis intentional and follows the established pattern in this codebase for providing installation-time shims. The identical implementation ensures consistent behavior regardless of how the tool is accessed.test/configDiffer/pathNormalizer.test.js (1)
1-129: LGTM! Comprehensive test coverage for PathNormalizer.The test suite provides excellent coverage:
- normalize() tests cover various scenarios: absolute paths, non-paths, nested structures, arrays, paths outside base, and relative paths
- detectBasePath() tests handle multiple paths, absence of paths, and single-path scenarios
- Clear, descriptive test names make the suite maintainable
- Good balance of happy-path and edge-case testing
The tests effectively validate the path normalization functionality that's central to the diff engine's ability to compare configurations across different environments.
package/configDiffer/diffEngine.ts (9)
13-23: LGTM! Well-structured initialization.The constructor properly sets sensible defaults for all options using nullish coalescing, making the API ergonomic for consumers.
25-39: LGTM! Clean public API.The
comparemethod correctly resets state, performs the diff, and returns a well-structured result with metadata including timestamp.
41-96: LGTM! Solid recursive comparison logic.The traversal correctly handles:
- Path ignoring and max depth constraints
- Undefined values (added/removed)
- Primitive comparisons with equality checks
- Array and object recursion
- Fallback for edge cases
The logic flow is clear and comprehensive.
98-128: LGTM! Array and object comparison is correct.Both methods properly:
- Enumerate all keys/indices
- Skip ignored keys
- Build paths accurately (
[i]for arrays, key names for objects)- Recurse with incremented depth
130-153: LGTM! Entry creation and path formatting.The methods correctly serialize values, format paths with both array and human-readable representations, and build entries with appropriate metadata.
184-224: Good type guards with appropriate equality checks.The type guards correctly classify values, and
areEqualhandles special cases for functions, RegExp, and Date. Note that comparing functions viatoString()(line 212) is fragile—it can produce false positives if functions differ only in formatting or variable names. However, for a configuration diff tool, this is likely acceptable since config functions are typically small and stable.
226-267: LGTM! Type inference and value serialization.Both methods correctly handle special types and provide appropriate string representations for display. The function length threshold (200 chars) and string truncation are reasonable heuristics.
269-305: LGTM! Summary calculation is correct.The method properly aggregates counts by operation type, excludes
unchangedfromtotalChanges, and conditionally includes theunchangedproperty based on options.Minor note: Line 292's
|| 0is unnecessary sinceunchangedis initialized to 0 on line 281, but it's harmless.
170-182: Ignore bracket-notation concern. createHumanPath joins all segments with the configured separator (default “.”), so an array segment “[0]” becomes “foo.[0]”, andstartsWith("foo.")correctly matches.Likely an incorrect or invalid review comment.
package/configDiffer/formatter.ts (6)
1-7: LGTM! Clean JSON formatting.Simple and effective—uses standard
JSON.stringifywith 2-space indentation for readability.
9-22: LGTM! Proper YAML formatting with correct js-yaml API.The YAML formatter:
- Uses
dump(the safe, recommended API in js-yaml 4.x)- Groups entries by operation for structured output
- Configures appropriate options (
noRefsprevents circular reference markers,sortKeys: falsepreserves insertion order)Based on learnings
24-47: LGTM! Clear summary output.The summary formatter provides a concise, readable overview with:
- Prominent section headers
- Aligned operation counts
- Special handling for zero changes
- Optional unchanged count
49-106: LGTM! Comprehensive detailed output.The detailed formatter builds a full report with:
- Metadata (timestamp, file paths)
- Embedded summary
- Grouped changes by operation (added, removed, changed)
- Per-entry formatting with clear section markers
The structure is well-organized and human-readable.
108-144: LGTM! Entry and value formatting.Both methods correctly:
- Format entry details (path, type, values) with consistent indentation
- Handle operation-specific value display (added shows newValue, removed shows oldValue, changed shows both)
- Truncate long strings at 100 chars for readability
- Handle primitive types and fall back to JSON.stringify
146-161: LGTM! Operation grouping logic.The method correctly initializes all operation types and populates them via iteration, ensuring the return type matches
Record<DiffOperation, DiffEntry[]>.
Updated based on feedbackCode Review Fixes
Format ImprovementsReplaced the generic summary/detailed formats with a contextual format that provides real value: Before (useless): After (useful): New Features
Why This is BetterThe new format answers:
This transforms the diff from "here's what changed" into "here's what changed AND what it means for your app." |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
package/configDiffer/diffEngine.ts (3)
177-187: Functions and RegExp classified as primitives may cause confusion.The
isPrimitivemethod treats functions and RegExp instances as primitives. While this makes sense for the diff logic (they're compared by string representation), it's semantically unusual and could be confusing for maintainers. Consider renaming toshouldCompareByValueor adding a comment explaining the rationale.Add a clarifying comment:
private isPrimitive(value: any): boolean { + // Note: Functions and RegExp are treated as "primitives" because they're + // compared by their string representation rather than recursively return ( value === null || typeof value === "string" || typeof value === "number" || typeof value === "boolean" || typeof value === "undefined" || typeof value === "function" || value instanceof RegExp ) }
199-217: Function equality comparison may produce false negatives.Comparing functions via
toString()can fail for functions with different formatting but identical behavior (e.g., minified vs non-minified). This is acceptable for config comparison but could be noted in documentation.Consider adding a comment to clarify the limitations:
private areEqual(left: any, right: any): boolean { if (left === right) { return true } + // Note: Function comparison uses toString(), which is sensitive to formatting + // but sufficient for detecting meaningful config changes if (typeof left === "function" && typeof right === "function") { return left.toString() === right.toString() }
229-260: Potential information disclosure in function serialization.When a function's string representation exceeds 200 characters, the code shows the character count. This could inadvertently reveal information about complex internal logic. Consider whether this level of detail is necessary or if a simpler representation would suffice.
Consider simplifying the output:
if (typeof value === "function") { const fnStr = value.toString() - if (fnStr.length > 200) { - return `[Function: ${value.name || "anonymous"}] (${fnStr.length} chars)` - } - return `[Function: ${value.name || "anonymous"}]` + return `[Function: ${value.name || "anonymous"}]` }package/configDiffer/cli.ts (1)
172-181: TypeScript support requires ts-node as peer dependency.The code attempts to require
ts-node/register/transpile-onlybut doesn't verify the version or show alternative installation methods. Users might install incompatible versions or be confused about whether it's a dev or peer dependency.Consider improving the error message with more context:
if (ext === ".ts") { try { require("ts-node/register/transpile-only") } catch (error) { throw new Error( "TypeScript config detected but ts-node is not available. " + - "Install ts-node as a dev dependency: npm install --save-dev ts-node" + "Install ts-node as a dev dependency: npm install --save-dev ts-node\n" + + "Alternatively, export your TypeScript config as JSON or YAML." ) } }package/configDiffer/formatter.ts (3)
167-173: Type coercion in boolean comparison may cause issues.Line 168 checks
newVal === true || newVal === "true", which handles both boolean and string representations. However, this may not catch other truthy values like1or"1"that could appear in configs. Consider normalizing to boolean first.if (path === "optimization.minimize") { - if (newVal === true || newVal === "true") { + const isEnabled = newVal === true || newVal === "true" || newVal === 1 || newVal === "1" + if (isEnabled) { return "Code will be minified - smaller bundles but slower builds" } else { return "Minification disabled - larger bundles but faster builds" } }
180-186: String type coercion could throw on non-string values.Lines 181-182 call
String(newVal).includes()which could produce unexpected results ifnewValisnull,undefined, or an object. While it won't throw, it might give false positives (e.g.,String(null)returns"null"which could match if someone has a devtool option called "nulleval").Add type checking:
} else if ( - String(newVal).includes("cheap") || - String(newVal).includes("eval") + (typeof newVal === "string" && (newVal.includes("cheap") || newVal.includes("eval"))) ) { return "Faster builds with some debugging capability" }
215-230: Type assertion bypasses type safety.Line 229 uses a type assertion
as Record<DiffOperation, DiffEntry[]>after manually constructing the grouped object. While this works, the manual initialization on lines 218-223 is fragile - if new operation types are added toDiffOperation, this function would silently fail to include them.Consider a more type-safe approach:
private groupByOperation( entries: DiffEntry[] ): Record<DiffOperation, DiffEntry[]> { - const grouped: Record<string, DiffEntry[]> = { - added: [], - removed: [], - changed: [], - unchanged: [] - } + const operations: DiffOperation[] = ["added", "removed", "changed", "unchanged"] + const grouped: Record<DiffOperation, DiffEntry[]> = {} as Record<DiffOperation, DiffEntry[]> + + for (const op of operations) { + grouped[op] = [] + } for (const entry of entries) { grouped[entry.operation].push(entry) } - return grouped as Record<DiffOperation, DiffEntry[]> + return grouped }Alternatively, use a reduce for a more functional approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
package/configDiffer/cli.ts(1 hunks)package/configDiffer/configDocs.ts(1 hunks)package/configDiffer/diffEngine.ts(1 hunks)package/configDiffer/formatter.ts(1 hunks)package/configDiffer/pathNormalizer.ts(1 hunks)test/configDiffer/formatter.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package/configDiffer/pathNormalizer.ts
- test/configDiffer/formatter.test.js
🧰 Additional context used
🧬 Code graph analysis (3)
package/configDiffer/diffEngine.ts (1)
package/configDiffer/types.ts (5)
DiffOptions(34-42)DiffEntry(8-14)DiffResult(16-32)DiffOperation(1-1)DiffPath(3-6)
package/configDiffer/cli.ts (4)
package/configDiffer/types.ts (1)
DiffOptions(34-42)package/configDiffer/pathNormalizer.ts (1)
PathNormalizer(4-167)package/configDiffer/diffEngine.ts (1)
DiffEngine(9-299)package/configDiffer/formatter.ts (1)
DiffFormatter(5-231)
package/configDiffer/formatter.ts (2)
package/configDiffer/types.ts (3)
DiffResult(16-32)DiffEntry(8-14)DiffOperation(1-1)package/configDiffer/configDocs.ts (1)
getDocForKey(161-163)
🔇 Additional comments (8)
package/configDiffer/configDocs.ts (3)
4-9: LGTM! Clean interface definition.The
ConfigDocinterface is well-structured with appropriate optional fields for documentation metadata.
161-167: LGTM! Simple and effective helper functions.The
getDocForKeyandhasDocumentationfunctions provide straightforward access to the documentation map.
11-159: Documentation URLs verified: All webpack documentation links return HTTP 200 and are valid.package/configDiffer/diffEngine.ts (2)
9-23: LGTM! Constructor properly initializes options with sensible defaults.The options are correctly typed as
Required<DiffOptions>and all optional fields are handled with nullish coalescing, ensuring consistent behavior.
25-39: LGTM! Clean comparison entry point with proper state management.The
comparemethod correctly resets the entries array before each comparison and includes metadata with timestamps.package/configDiffer/cli.ts (1)
16-89: LGTM! Previous review comment addressed - path normalization now uses separate base paths.The
runfunction correctly normalizes each configuration with its own detected base path (lines 38-49), addressing the previous review concern about cross-environment path normalization.package/configDiffer/formatter.ts (2)
1-8: LGTM! Clean imports and simple JSON formatter.The
formatJsonmethod provides a straightforward JSON representation with proper indentation.
10-23: LGTM! YAML formatting uses safe defaults.The YAML formatter uses
js-yamlwith appropriate options:noRefs: trueprevents circular reference issues,sortKeys: falsepreserves order, andlineWidth: 120ensures readability.Based on learnings: js-yaml v4.x uses safe-by-default
dump, which is correctly used here.
| if (options.output) { | ||
| const fs = require("fs") | ||
| fs.writeFileSync(options.output, output, "utf8") | ||
| console.log(`Diff written to: ${options.output}`) | ||
| } else { | ||
| console.log(output) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid dynamic require() inside conditional block.
The require('fs') call on line 77 is redundant since fs is already imported at the top of the file. This also creates inconsistency - other parts of the code use the named import writeFileSync would be cleaner.
Use the existing import:
+import { existsSync, readFileSync, writeFileSync } from "fs"
import { resolve, extname } from "path"
import { load as loadYaml } from "js-yaml"
...
if (options.output) {
- const fs = require("fs")
- fs.writeFileSync(options.output, output, "utf8")
+ writeFileSync(options.output, output, "utf8")
console.log(`Diff written to: ${options.output}`)
} else {📝 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.
| if (options.output) { | |
| const fs = require("fs") | |
| fs.writeFileSync(options.output, output, "utf8") | |
| console.log(`Diff written to: ${options.output}`) | |
| } else { | |
| console.log(output) | |
| } | |
| // At the top of package/configDiffer/cli.ts | |
| import { existsSync, readFileSync, writeFileSync } from "fs" | |
| import { resolve, extname } from "path" | |
| import { load as loadYaml } from "js-yaml" | |
| … | |
| if (options.output) { | |
| writeFileSync(options.output, output, "utf8") | |
| console.log(`Diff written to: ${options.output}`) | |
| } else { | |
| console.log(output) | |
| } |
🤖 Prompt for AI Agents
In package/configDiffer/cli.ts around lines 76 to 82, remove the dynamic
require("fs") and the local fs variable and instead use the existing top-level
fs (or the named writeFileSync import) already imported at the top of the file;
replace fs.writeFileSync(options.output, output, "utf8") with the existing
writeFileSync(...) call (or fs.writeFileSync if using the fs default import) and
keep the console.log message unchanged.
| delete require.cache[resolvedPath] | ||
| let loaded = require(resolvedPath) | ||
|
|
||
| if (typeof loaded === "object" && "default" in loaded) { | ||
| loaded = loaded.default | ||
| } | ||
|
|
||
| return loaded |
There was a problem hiding this comment.
Require cache clearing may not work for ES modules.
Line 183 clears the require cache to support reloading configs, but this only works for CommonJS modules. If the loaded JS/TS file uses ES modules, this won't have the expected effect and could lead to stale config being used on subsequent runs.
Document this limitation or add detection:
delete require.cache[resolvedPath]
let loaded = require(resolvedPath)
+ // Note: Cache clearing only works for CommonJS modules.
+ // For ES modules, configs may not reload properly in the same process.
+
if (typeof loaded === "object" && "default" in loaded) {
loaded = loaded.default
}📝 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.
| delete require.cache[resolvedPath] | |
| let loaded = require(resolvedPath) | |
| if (typeof loaded === "object" && "default" in loaded) { | |
| loaded = loaded.default | |
| } | |
| return loaded | |
| delete require.cache[resolvedPath] | |
| let loaded = require(resolvedPath) | |
| // Note: Cache clearing only works for CommonJS modules. | |
| // For ES modules, configs may not reload properly in the same process. | |
| if (typeof loaded === "object" && "default" in loaded) { | |
| loaded = loaded.default | |
| } | |
| return loaded |
| private shouldIgnorePath(path: string[]): boolean { | ||
| const humanPath = this.createHumanPath(path) | ||
| return this.options.ignorePaths.some((ignorePath) => { | ||
| if (ignorePath.includes("*")) { | ||
| const escapedPattern = ignorePath | ||
| .replace(/\./g, "\\.") | ||
| .replace(/\*/g, ".*") | ||
| const pattern = new RegExp("^" + escapedPattern + "$") | ||
| return pattern.test(humanPath) | ||
| } | ||
| return humanPath === ignorePath || humanPath.startsWith(ignorePath + ".") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Potential regex denial-of-service (ReDoS) vulnerability.
The wildcard pattern conversion creates a regex with .* which can lead to catastrophic backtracking on certain inputs. If a user provides a malicious ignore pattern with nested wildcards or very long paths, this could cause the comparison to hang.
Consider adding a timeout or using a simpler matching algorithm:
private shouldIgnorePath(path: string[]): boolean {
const humanPath = this.createHumanPath(path)
return this.options.ignorePaths.some((ignorePath) => {
if (ignorePath.includes("*")) {
- const escapedPattern = ignorePath
- .replace(/\./g, "\\.")
- .replace(/\*/g, ".*")
- const pattern = new RegExp("^" + escapedPattern + "$")
- return pattern.test(humanPath)
+ // Use a simpler glob-like matching to avoid ReDoS
+ const parts = ignorePath.split("*")
+ let position = 0
+ for (let i = 0; i < parts.length; i++) {
+ const part = parts[i].replace(/\./g, "\\.")
+ if (part.length > 0) {
+ const index = humanPath.indexOf(part, position)
+ if (index === -1) return false
+ if (i === 0 && index !== 0) return false
+ position = index + part.length
+ }
+ }
+ if (parts.length > 0 && parts[parts.length - 1].length > 0) {
+ return humanPath.endsWith(parts[parts.length - 1])
+ }
+ return true
}
return humanPath === ignorePath || humanPath.startsWith(ignorePath + ".")
})
}Committable suggestion skipped: line range outside the PR's diff.
| if (path.includes("output.filename")) { | ||
| if (String(newVal).includes("[contenthash]")) { | ||
| return "Cache busting enabled - better long-term caching" | ||
| } | ||
| } |
There was a problem hiding this comment.
Path.includes() check could match unintended paths.
Line 188 uses path.includes("output.filename") which will match any path containing that substring, not just the exact key. For example, it would match "some.output.filename.custom" or "output.filename.template".
Use exact or prefix matching:
- if (path.includes("output.filename")) {
+ if (path === "output.filename" || path.startsWith("output.filename.")) {
if (String(newVal).includes("[contenthash]")) {
return "Cache busting enabled - better long-term caching"
}
}🤖 Prompt for AI Agents
In package/configDiffer/formatter.ts around lines 188 to 192, the
path.includes("output.filename") check can match unintended keys (any path
containing that substring); replace it with a precise match such as path ===
"output.filename" or a proper prefix/segment check (e.g., split on '.' and
compare the relevant segment or use startsWith("output.filename.") when matching
nested keys) so only the exact key or intended prefix triggers the cache-busting
message; update the conditional accordingly and keep the inner contenthash check
as-is.
Further improvements based on feedbackRemoved redundant information
New label extractionThe formatter now intelligently extracts short names from filenames:
Improved output formatBefore: After: Full exampleMuch clearer! You can immediately see which config has which value. Future-proof for N-way comparisonThis format naturally extends to comparing 3+ configs: The design is ready for when we want to add multi-file comparison support. |
justin808
force-pushed
the
config-diff-engine
branch
from
3d27583 to
8ad0017
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
package/configDiffer/formatter.ts (1)
246-250: Matchoutput.filenameprecisely for impact hints.The
includescheck catches any path containing that substring (e.g.,optimization.output.filenameTemplate), leading to misleading cache-busting advice. Compare the path segments directly instead:- if (path.includes("output.filename")) { + const segments = entry.path.path + if ( + segments.length >= 2 && + segments[0] === "output" && + segments[1] === "filename" + ) {package/configDiffer/cli.ts (2)
183-190: Document require cache limitation for ES modules.The
require.cacheclearing on line 183 only works for CommonJS modules. ES modules loaded in the same process won't reload, potentially causing stale config to be used.Add a comment documenting this limitation:
delete require.cache[resolvedPath] + // Note: Cache clearing only works for CommonJS modules. + // For ES modules, configs may not reload properly in the same process. let loaded = require(resolvedPath) if (typeof loaded === "object" && "default" in loaded) {
1-1: AddwriteFileSyncto import and remove dynamicrequire.The dynamic
require('fs')on line 77 is inconsistent with the top-level import on line 1. To resolve this, addwriteFileSyncto the named imports and use it directly.Apply this diff:
-import { existsSync, readFileSync } from "fs" +import { existsSync, readFileSync, writeFileSync } from "fs" import { resolve, extname } from "path" import { load as loadYaml } from "js-yaml"Then remove the dynamic require:
if (options.output) { - const fs = require("fs") - fs.writeFileSync(options.output, output, "utf8") + writeFileSync(options.output, output, "utf8") console.log(`Diff written to: ${options.output}`) } else {Also applies to: 76-79
🧹 Nitpick comments (1)
docs/config-diff.md (1)
329-351: Annotate the command reference code block.Please specify a language (e.g.,
textorbash) for this fenced block so markdownlint stops flagging MD040 and downstream renderers get proper formatting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
README.md(1 hunks)bin/diff-bundler-config(1 hunks)docs/config-diff.md(1 hunks)lib/install/bin/diff-bundler-config(1 hunks)package.json(1 hunks)package/configDiffer/cli.ts(1 hunks)package/configDiffer/configDocs.ts(1 hunks)package/configDiffer/diffEngine.ts(1 hunks)package/configDiffer/formatter.ts(1 hunks)package/configDiffer/index.ts(1 hunks)package/configDiffer/pathNormalizer.ts(1 hunks)package/configDiffer/types.ts(1 hunks)test/configDiffer/diffEngine.test.js(1 hunks)test/configDiffer/formatter.test.js(1 hunks)test/configDiffer/pathNormalizer.test.js(1 hunks)tmp/examples/dev-config.yaml(1 hunks)tmp/examples/prod-config.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- package/configDiffer/configDocs.ts
- tmp/examples/dev-config.yaml
- bin/diff-bundler-config
- test/configDiffer/diffEngine.test.js
- package/configDiffer/index.ts
- lib/install/bin/diff-bundler-config
- test/configDiffer/formatter.test.js
- package.json
- package/configDiffer/types.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
End every file with a trailing newline character
Files:
package/configDiffer/formatter.tspackage/configDiffer/diffEngine.tsREADME.mdpackage/configDiffer/cli.tstest/configDiffer/pathNormalizer.test.jstmp/examples/prod-config.yamlpackage/configDiffer/pathNormalizer.tsdocs/config-diff.md
🧬 Code graph analysis (5)
package/configDiffer/formatter.ts (2)
package/configDiffer/types.ts (3)
DiffResult(16-32)DiffEntry(8-14)DiffOperation(1-1)package/configDiffer/configDocs.ts (1)
getDocForKey(161-163)
package/configDiffer/diffEngine.ts (1)
package/configDiffer/types.ts (5)
DiffOptions(34-42)DiffEntry(8-14)DiffResult(16-32)DiffOperation(1-1)DiffPath(3-6)
package/configDiffer/cli.ts (4)
package/configDiffer/types.ts (1)
DiffOptions(34-42)package/configDiffer/pathNormalizer.ts (1)
PathNormalizer(4-167)package/configDiffer/diffEngine.ts (1)
DiffEngine(9-299)package/configDiffer/formatter.ts (1)
DiffFormatter(5-289)
test/configDiffer/pathNormalizer.test.js (1)
package/configDiffer/pathNormalizer.ts (1)
PathNormalizer(4-167)
package/configDiffer/pathNormalizer.ts (2)
package/configDiffer/index.ts (2)
PathNormalizer(4-4)NormalizedConfig(11-11)package/configDiffer/types.ts (1)
NormalizedConfig(44-48)
🪛 markdownlint-cli2 (0.18.1)
docs/config-diff.md
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
15-15: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
27-27: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
34-34: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
329-329: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (11)
test/configDiffer/pathNormalizer.test.js (2)
3-90: LGTM!The normalize test suite provides comprehensive coverage of the PathNormalizer's core functionality: absolute-to-relative conversion, non-path preservation, nested object/array handling, and edge cases. The test structure is clear and validates expected behavior effectively.
92-128: LGTM!The detectBasePath test suite effectively validates the static method's behavior across key scenarios: deriving common prefixes from multiple paths, handling configs without paths, and single-path configs. The assertions appropriately account for platform path differences.
package/configDiffer/cli.ts (2)
38-49: LGTM!The path normalization correctly uses independent base paths for left and right configs, addressing the past review concern about cross-environment path differences. Each config now normalizes paths relative to its own detected base, ensuring consistent relative paths across machines.
91-155: LGTM!The CLI implementation is robust:
parseArgumentscomprehensively handles all flags with proper validation and clear error messagesloadConfigFilesupports multiple formats (JSON, YAML, JS, TS) with appropriate error handling and ts-node integrationshowHelpprovides detailed usage documentation with practical examples and exit code descriptionsThe implementation follows best practices for CLI design.
Also applies to: 157-182, 191-260
package/configDiffer/pathNormalizer.ts (7)
4-17: LGTM!The constructor and
normalizemethod are well-designed. The optionalbasePathparameter with a sensible default (process.cwd()) provides flexibility, and the returnedNormalizedConfigstructure preserves both original and normalized configs along with the base path used.
19-39: LGTM!The
normalizeValuemethod correctly handles recursive normalization of nested structures. The use ofisPlainObjectprevents incorrect traversal of special types (Date, RegExp, functions) while properly normalizing strings, arrays, and plain objects.
41-55: LGTM!The
normalizePathmethod implements correct path normalization logic:
- Early return for non-path strings avoids unnecessary processing
- Converts paths to relative form with "./" prefix for consistency
- Preserves paths outside the base path (starting with "..") unchanged
- Normalizes separators to "/" for cross-platform consistency
57-99: LGTM!The
looksLikePathmethod now correctly restricts path detection to actual filesystem paths, addressing the past review concern. The implementation:
- Excludes URLs with schemes (http://, https://, webpack://, etc.) via regex on line 63
- Excludes module specifiers starting with @ on line 68
- Only matches filesystem path patterns: absolute paths, relative prefixes (./, ../), home paths (~/, and Windows drive paths
This prevents false positives on URLs and module specifiers, ensuring only legitimate filesystem paths are normalized.
101-119: LGTM!The
isPlainObjectmethod correctly identifies plain objects by excluding null, primitives, arrays, Date, RegExp, and functions. This ensures special types aren't incorrectly traversed during normalization.
121-142: LGTM!The
detectBasePathstatic method correctly extracts absolute paths from configs and derives a common base path. Returningundefinedwhen no paths exist allows callers to fall back toprocess.cwd(), providing sensible behavior for configs without filesystem paths.
144-166: LGTM!The
findCommonPrefixstatic method correctly handles edge cases and derives the common directory prefix:
- Empty array returns empty string
- Single path returns its directory (filename removed)
- Multiple paths find the longest common prefix by comparing path segments
The implementation is robust and handles cross-platform separators correctly.
| if (this.options.maxDepth !== null && depth > this.options.maxDepth) { | ||
| return | ||
| } | ||
|
|
||
| const leftType = this.getValueType(left) | ||
| const rightType = this.getValueType(right) | ||
|
|
||
| if (left === undefined && right === undefined) { | ||
| return | ||
| } | ||
|
|
||
| if (left === undefined) { | ||
| this.addEntry("added", path, undefined, right, rightType) | ||
| return | ||
| } | ||
|
|
||
| if (right === undefined) { | ||
| this.addEntry("removed", path, left, undefined, leftType) | ||
| return | ||
| } | ||
|
|
||
| if (this.isPrimitive(left) || this.isPrimitive(right)) { | ||
| if (!this.areEqual(left, right)) { | ||
| this.addEntry("changed", path, left, right, leftType) | ||
| } else if (this.options.includeUnchanged) { | ||
| this.addEntry("unchanged", path, left, right, leftType) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if (Array.isArray(left) && Array.isArray(right)) { | ||
| this.compareArrays(left, right, path, depth) | ||
| return | ||
| } | ||
|
|
||
| if (this.isObject(left) && this.isObject(right)) { | ||
| this.compareObjects(left, right, path, depth) | ||
| return | ||
| } | ||
|
|
||
| if (!this.areEqual(left, right)) { | ||
| this.addEntry("changed", path, left, right, leftType) | ||
| } else if (this.options.includeUnchanged) { | ||
| this.addEntry("unchanged", path, left, right, leftType) | ||
| } | ||
| } |
There was a problem hiding this comment.
Honor maxDepth without hiding differences.
When maxDepth is reached (e.g., value 0 or 1), the early return prevents any entry from being recorded, so configurations can appear identical even though deeper nodes differ. We still need an aggregated added/removed/changed (or unchanged) entry at the current path before exiting. Apply this fix:
- if (this.options.maxDepth !== null && depth > this.options.maxDepth) {
- return
- }
-
- const leftType = this.getValueType(left)
- const rightType = this.getValueType(right)
+ const leftType = this.getValueType(left)
+ const rightType = this.getValueType(right)
+
+ if (this.options.maxDepth !== null && depth > this.options.maxDepth) {
+ if (left === undefined && right !== undefined) {
+ this.addEntry("added", path, undefined, right, rightType)
+ } else if (right === undefined && left !== undefined) {
+ this.addEntry("removed", path, left, undefined, leftType)
+ } else if (!this.areEqual(left, right)) {
+ this.addEntry("changed", path, left, right, leftType)
+ } else if (this.options.includeUnchanged) {
+ this.addEntry("unchanged", path, left, right, leftType)
+ }
+ return
+ }📝 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.
| if (this.options.maxDepth !== null && depth > this.options.maxDepth) { | |
| return | |
| } | |
| const leftType = this.getValueType(left) | |
| const rightType = this.getValueType(right) | |
| if (left === undefined && right === undefined) { | |
| return | |
| } | |
| if (left === undefined) { | |
| this.addEntry("added", path, undefined, right, rightType) | |
| return | |
| } | |
| if (right === undefined) { | |
| this.addEntry("removed", path, left, undefined, leftType) | |
| return | |
| } | |
| if (this.isPrimitive(left) || this.isPrimitive(right)) { | |
| if (!this.areEqual(left, right)) { | |
| this.addEntry("changed", path, left, right, leftType) | |
| } else if (this.options.includeUnchanged) { | |
| this.addEntry("unchanged", path, left, right, leftType) | |
| } | |
| return | |
| } | |
| if (Array.isArray(left) && Array.isArray(right)) { | |
| this.compareArrays(left, right, path, depth) | |
| return | |
| } | |
| if (this.isObject(left) && this.isObject(right)) { | |
| this.compareObjects(left, right, path, depth) | |
| return | |
| } | |
| if (!this.areEqual(left, right)) { | |
| this.addEntry("changed", path, left, right, leftType) | |
| } else if (this.options.includeUnchanged) { | |
| this.addEntry("unchanged", path, left, right, leftType) | |
| } | |
| } | |
| const leftType = this.getValueType(left) | |
| const rightType = this.getValueType(right) | |
| if (this.options.maxDepth !== null && depth > this.options.maxDepth) { | |
| if (left === undefined && right !== undefined) { | |
| this.addEntry("added", path, undefined, right, rightType) | |
| } else if (right === undefined && left !== undefined) { | |
| this.addEntry("removed", path, left, undefined, leftType) | |
| } else if (!this.areEqual(left, right)) { | |
| this.addEntry("changed", path, left, right, leftType) | |
| } else if (this.options.includeUnchanged) { | |
| this.addEntry("unchanged", path, left, right, leftType) | |
| } | |
| return | |
| } | |
| if (left === undefined && right === undefined) { | |
| return | |
| } | |
| if (left === undefined) { | |
| this.addEntry("added", path, undefined, right, rightType) | |
| return | |
| } | |
| if (right === undefined) { | |
| this.addEntry("removed", path, left, undefined, leftType) | |
| return | |
| } | |
| if (this.isPrimitive(left) || this.isPrimitive(right)) { | |
| if (!this.areEqual(left, right)) { | |
| this.addEntry("changed", path, left, right, leftType) | |
| } else if (this.options.includeUnchanged) { | |
| this.addEntry("unchanged", path, left, right, leftType) | |
| } | |
| return | |
| } | |
| if (Array.isArray(left) && Array.isArray(right)) { | |
| this.compareArrays(left, right, path, depth) | |
| return | |
| } | |
| if (this.isObject(left) && this.isObject(right)) { | |
| this.compareObjects(left, right, path, depth) | |
| return | |
| } | |
| if (!this.areEqual(left, right)) { | |
| this.addEntry("changed", path, left, right, leftType) | |
| } else if (this.options.includeUnchanged) { | |
| this.addEntry("unchanged", path, left, right, leftType) | |
| } |
| private shouldIgnorePath(path: string[]): boolean { | ||
| const humanPath = this.createHumanPath(path) | ||
| return this.options.ignorePaths.some((ignorePath) => { | ||
| if (ignorePath.includes("*")) { | ||
| const escapedPattern = ignorePath | ||
| .replace(/\./g, "\\.") | ||
| .replace(/\*/g, ".*") | ||
| const pattern = new RegExp("^" + escapedPattern + "$") | ||
| return pattern.test(humanPath) | ||
| } | ||
| return humanPath === ignorePath || humanPath.startsWith(ignorePath + ".") | ||
| }) |
There was a problem hiding this comment.
Fix wildcard ignore matching and respect custom separators.
Replacing only dots and stars leaves characters like [ and ] unescaped, so patterns such as module.rules.[*] become module\.rules\.[.*] and never match array paths. The literal "." in ignorePath + "." also breaks when callers use a custom pathSeparator. Please escape all regex metacharacters before expanding wildcards and build the prefix check with the configured separator instead:
- return this.options.ignorePaths.some((ignorePath) => {
- if (ignorePath.includes("*")) {
- const escapedPattern = ignorePath
- .replace(/\./g, "\\.")
- .replace(/\*/g, ".*")
- const pattern = new RegExp("^" + escapedPattern + "$")
- return pattern.test(humanPath)
- }
- return humanPath === ignorePath || humanPath.startsWith(ignorePath + ".")
+ const separator = this.options.pathSeparator
+ return this.options.ignorePaths.some((ignorePath) => {
+ if (ignorePath.includes("*")) {
+ const escapedSegments = ignorePath
+ .split("*")
+ .map((segment) =>
+ segment.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")
+ )
+ const pattern = new RegExp(
+ "^" + escapedSegments.join(".*") + "$"
+ )
+ return pattern.test(humanPath)
+ }
+ return (
+ humanPath === ignorePath ||
+ humanPath.startsWith(ignorePath + separator)
+ )
})📝 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.
| private shouldIgnorePath(path: string[]): boolean { | |
| const humanPath = this.createHumanPath(path) | |
| return this.options.ignorePaths.some((ignorePath) => { | |
| if (ignorePath.includes("*")) { | |
| const escapedPattern = ignorePath | |
| .replace(/\./g, "\\.") | |
| .replace(/\*/g, ".*") | |
| const pattern = new RegExp("^" + escapedPattern + "$") | |
| return pattern.test(humanPath) | |
| } | |
| return humanPath === ignorePath || humanPath.startsWith(ignorePath + ".") | |
| }) | |
| private shouldIgnorePath(path: string[]): boolean { | |
| const humanPath = this.createHumanPath(path) | |
| const separator = this.options.pathSeparator | |
| return this.options.ignorePaths.some((ignorePath) => { | |
| if (ignorePath.includes("*")) { | |
| const escapedSegments = ignorePath | |
| .split("*") | |
| .map((segment) => | |
| segment.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") | |
| ) | |
| const pattern = new RegExp( | |
| "^" + escapedSegments.join(".*") + "$" | |
| ) | |
| return pattern.test(humanPath) | |
| } | |
| return ( | |
| humanPath === ignorePath || | |
| humanPath.startsWith(ignorePath + separator) | |
| ) | |
| }) | |
| } |
justin808
force-pushed
the
config-diff-engine
branch
2 times, most recently
from
155f920 to
0633d0d
Compare
justin808
force-pushed
the
config-diff-engine
branch
from
0633d0d to
a2e4bdd
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
package/configDiffer/diffEngine.ts (1)
163-175: Robust wildcard ignores: escape meta, honor custom separator, and reduce ReDoS riskCurrent logic:
- Escapes only “.” and “*”; brackets and other metacharacters break patterns like
module.rules.[*].- Hardcodes ".", ignoring
pathSeparator.- Regex
.*can be abused for backtracking.Prefer a safe glob or, at minimum, fully escape segments and respect
pathSeparator.private shouldIgnorePath(path: string[]): boolean { const humanPath = this.createHumanPath(path) - return this.options.ignorePaths.some((ignorePath) => { - if (ignorePath.includes("*")) { - const escapedPattern = ignorePath - .replace(/\./g, "\\.") - .replace(/\*/g, ".*") - const pattern = new RegExp("^" + escapedPattern + "$") - return pattern.test(humanPath) - } - return humanPath === ignorePath || humanPath.startsWith(ignorePath + ".") - }) + const sep = this.options.pathSeparator + return this.options.ignorePaths.some((ignorePath) => { + if (ignorePath.includes("*")) { + const escapedSegments = ignorePath + .split("*") + .map((segment) => segment.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")) + const pattern = new RegExp("^" + escapedSegments.join(".*") + "$") + return pattern.test(humanPath) + } + return humanPath === ignorePath || humanPath.startsWith(ignorePath + sep) + }) }Alternatively, replace the regex with a small glob scan to avoid ReDoS entirely.
package/configDiffer/formatter.ts (1)
246-250: Tighten key match for output.filename to avoid false positives.Use exact or prefix segment match instead of substring includes. This was flagged earlier; applying here prevents unintended matches.
- if (path.includes("output.filename")) { + if (path === "output.filename" || path.startsWith("output.filename.")) { if (String(newVal).includes("[contenthash]")) { return "Cache busting enabled - better long-term caching" } }
🧹 Nitpick comments (8)
bin/diff-bundler-config (1)
7-11: Differentiate “diff found” vs “CLI error” and print robust errorsUse exit code 2 for unexpected failures and surface stack when available. This makes CI logic reliable and debuggable.
run(process.argv.slice(2)) - .then((exitCode) => process.exit(exitCode)) - .catch((error) => { - console.error(error.message) - process.exit(1) - }) + .then((code) => process.exit(code ?? 0)) + .catch((err) => { + const msg = err && (err.stack || err.message) ? (err.stack || err.message) : String(err) + console.error(msg) + process.exit(2) + })docs/config-diff.md (3)
9-39: Use proper headings instead of bold to satisfy markdownlint and improve structureConvert emphatic lines to headings.
-**Configuration-Specific Intelligence** +### Configuration-specific intelligence ... -**Path Normalization** +### Path normalization ... -**Focused Analysis** +### Focused analysis ... -**Multiple Output Formats** +### Multiple output formats ... -**Semantic Understanding** +### Semantic understanding
329-351: Add language to fenced block (markdownlint MD040)Specify a language for the command reference example.
-``` +```bash bin/diff-bundler-config --left=<file1> --right=<file2> [options] Required: --left=<file> First (left) config file to compare --right=<file> Second (right) config file to compare
355-357: Recommend distinct exit code for errorsUsing the same code (1) for “differences” and “errors” makes CI brittle. Suggest: 0 = equal, 1 = differences, 2 = error. Update docs and CLI accordingly.
- - `1` - Differences found or error occurred + - `1` - Differences found + - `2` - Error occurredConfirm CLI returns 2 on errors (see bin change suggestion).
test/configDiffer/pathNormalizer.test.js (1)
20-32: Add coverage for URL/module-specifier exclusions and Windows pathsStrengthen guarantees around “exclude URLs and @-scoped specifiers” and OS variance.
describe("normalize", () => { + test("does not normalize URLs and module specifiers", () => { + const normalizer = new PathNormalizer("/app") + const config = { + devtool: "webpack://", + source: "http://example.com/file.js", + pkg: "@scope/package" + } + const result = normalizer.normalize(config) + expect(result.normalized.devtool).toBe("webpack://") + expect(result.normalized.source).toBe("http://example.com/file.js") + expect(result.normalized.pkg).toBe("@scope/package") + }) + + test("normalizes Windows drive paths to relative", () => { + const basePath = "C:\\app\\project" + const normalizer = new PathNormalizer(basePath) + const config = { out: "C:\\app\\project\\public\\packs" } + const result = normalizer.normalize(config) + expect(result.normalized.out).toBe("./public/packs") + })Also applies to: 67-77, 79-89
test/configDiffer/diffEngine.test.js (2)
120-140: Add tests for custom pathSeparator and bracket wildcards in ignorePathsEnsure ignores work when using non-dot separators and when matching array indices.
describe("options", () => { + test("respects custom pathSeparator in ignorePaths prefix check", () => { + const engine = new DiffEngine({ + pathSeparator: "/", + ignorePaths: ["plugins/*"] + }) + const result = engine.compare( + { plugins: { a: 1 }, other: 1 }, + { plugins: { a: 2 }, other: 2 } + ) + expect(result.summary.totalChanges).toBe(1) + expect(result.entries[0].path.humanPath).toBe("other") + }) + + test("supports bracket wildcard for array indices", () => { + const engine = new DiffEngine({ ignorePaths: ["rules.[*]"] }) + const result = engine.compare( + { rules: [{ test: "a" }, { test: "b" }], x: 1 }, + { rules: [{ test: "A" }, { test: "B" }], x: 2 } + ) + expect(result.summary.totalChanges).toBe(1) + expect(result.entries[0].path.humanPath).toBe("x") + })Also applies to: 142-151
153-171: Add test to catch valueType bug when newValue is falsy but type changesPrevents regressions around
valueTypeinference.describe("special types", () => { + test("valueType reflects newValue even when falsy", () => { + const engine = new DiffEngine() + const result = engine.compare({ a: "1" }, { a: 0 }) + expect(result.entries[0].operation).toBe("changed") + expect(result.entries[0].valueType).toBe("number") + })test/configDiffer/formatter.test.js (1)
91-129: Add direct tests for formatContextual (default CLI format).We only test formatDetailed (alias) but not formatContextual explicitly. Add coverage to lock headers, legend, labels, and entry rendering.
@@ describe("formatDetailed", () => { @@ }) + + describe("formatContextual", () => { + test("renders header, comparing lines, legend, and entries", () => { + const formatter = new DiffFormatter() + const output = formatter.formatContextual(mockResult) + + expect(output).toContain("Webpack/Rspack Configuration Comparison") + expect(output).toContain("Comparing:") + expect(output).toContain("Legend:") + expect(output).toContain("[+]") + expect(output).toContain("[-]") + expect(output).toContain("[~]") + expect(output).toContain("newKey") + expect(output).toContain("oldKey") + expect(output).toContain("changedKey") + expect(output).toMatch(/Values:\s+.+:\s/) + }) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
README.md(1 hunks)bin/diff-bundler-config(1 hunks)docs/config-diff.md(1 hunks)lib/install/bin/diff-bundler-config(1 hunks)package.json(1 hunks)package/configDiffer/cli.ts(1 hunks)package/configDiffer/configDocs.ts(1 hunks)package/configDiffer/diffEngine.ts(1 hunks)package/configDiffer/formatter.ts(1 hunks)package/configDiffer/index.ts(1 hunks)package/configDiffer/pathNormalizer.ts(1 hunks)package/configDiffer/types.ts(1 hunks)test/configDiffer/diffEngine.test.js(1 hunks)test/configDiffer/formatter.test.js(1 hunks)test/configDiffer/pathNormalizer.test.js(1 hunks)tmp/examples/dev-config.yaml(1 hunks)tmp/examples/prod-config.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- package/configDiffer/configDocs.ts
- package.json
- package/configDiffer/index.ts
- lib/install/bin/diff-bundler-config
- package/configDiffer/types.ts
- tmp/examples/prod-config.yaml
- tmp/examples/dev-config.yaml
- package/configDiffer/cli.ts
- README.md
🧰 Additional context used
🧬 Code graph analysis (6)
test/configDiffer/diffEngine.test.js (1)
package/configDiffer/diffEngine.ts (1)
DiffEngine(9-299)
package/configDiffer/pathNormalizer.ts (2)
package/configDiffer/index.ts (2)
PathNormalizer(4-4)NormalizedConfig(11-11)package/configDiffer/types.ts (1)
NormalizedConfig(44-48)
package/configDiffer/diffEngine.ts (1)
package/configDiffer/types.ts (5)
DiffOptions(34-42)DiffEntry(8-14)DiffResult(16-32)DiffOperation(1-1)DiffPath(3-6)
package/configDiffer/formatter.ts (2)
package/configDiffer/types.ts (3)
DiffResult(16-32)DiffEntry(8-14)DiffOperation(1-1)package/configDiffer/configDocs.ts (1)
getDocForKey(161-163)
test/configDiffer/pathNormalizer.test.js (1)
package/configDiffer/pathNormalizer.ts (1)
PathNormalizer(4-167)
test/configDiffer/formatter.test.js (1)
package/configDiffer/formatter.ts (1)
DiffFormatter(5-289)
🪛 markdownlint-cli2 (0.18.1)
docs/config-diff.md
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
15-15: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
27-27: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
34-34: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
329-329: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
docs/config-diff.md (1)
216-226: Default output format: contextual vs detailed — please confirmDocs say “Detailed (default)”; PR summary mentions a new contextual default. DiffEngine currently defaults to "detailed". Align CLI behavior and docs.
Would you like the CLI default to be contextual? If yes, update both CLI help/docs and the DiffFormatter/CLI default accordingly. Based on learnings.
package/configDiffer/diffEngine.ts (1)
51-53: Max depth semantics: do you want an aggregated “elided” entry instead of hiding diffs?Early return makes deep differences invisible (summary=0). Consider recording an aggregated added/removed/changed (or unchanged when requested) at the current path when depth is exceeded.
If desired, I can provide a scoped patch that adds a single aggregated entry at
pathwhendepth > maxDepthwithout descending.
| oldValue: this.serializeValue(oldValue), | ||
| newValue: this.serializeValue(newValue), | ||
| valueType: valueType || this.getValueType(newValue || oldValue) | ||
| } |
There was a problem hiding this comment.
Fix valueType when newValue is falsy (e.g., 0, "", false)
newValue || oldValue misreports type. Use an undefined check.
- valueType: valueType || this.getValueType(newValue || oldValue)
+ valueType: valueType ?? this.getValueType(newValue !== undefined ? newValue : oldValue)📝 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.
| oldValue: this.serializeValue(oldValue), | |
| newValue: this.serializeValue(newValue), | |
| valueType: valueType || this.getValueType(newValue || oldValue) | |
| } | |
| oldValue: this.serializeValue(oldValue), | |
| newValue: this.serializeValue(newValue), | |
| valueType: valueType ?? this.getValueType(newValue !== undefined ? newValue : oldValue) | |
| } |
🤖 Prompt for AI Agents
In package/configDiffer/diffEngine.ts around lines 140 to 143, the expression
`newValue || oldValue` incorrectly treats valid falsy values (0, "", false) as
absent when determining valueType; change the selection to use an undefined
check (e.g., use `newValue === undefined ? oldValue : newValue`) so that only
undefined falls back to oldValue, and keep using the provided valueType when
present.
| const basename = filename.split("/").pop() || filename | ||
| const withoutExt = basename.replace(/\.(yaml|yml|json|js|ts)$/, "") | ||
|
|
There was a problem hiding this comment.
Cross‑platform filename parsing: support backslashes.
split("/") fails on Windows paths. Use a cross‑platform pattern.
- const basename = filename.split("/").pop() || filename
+ const basename = filename.split(/[\\/]/).pop() || filename📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const basename = filename.split("/").pop() || filename | |
| const withoutExt = basename.replace(/\.(yaml|yml|json|js|ts)$/, "") | |
| const basename = filename.split(/[\\/]/).pop() || filename | |
| const withoutExt = basename.replace(/\.(yaml|yml|json|js|ts)$/, "") |
🤖 Prompt for AI Agents
In package/configDiffer/formatter.ts around lines 97 to 99, filename parsing
uses split("/") which breaks on Windows paths; replace it with Node's
cross-platform path handling: import the built-in path module (e.g., const path
= require('path') or an ES import), call path.basename(filename) to obtain the
basename, then run the existing extension-stripping regex on that basename;
update imports accordingly and remove the split("/") usage.
| private normalizePath(str: string): string { | ||
| if (!this.looksLikePath(str)) { | ||
| return str | ||
| } | ||
|
|
||
| const absolutePath = isAbsolute(str) ? str : resolve(this.basePath, str) | ||
|
|
||
| const relativePath = relative(this.basePath, absolutePath) | ||
|
|
||
| if (relativePath && !relativePath.startsWith("..")) { | ||
| return "./" + relativePath.split(sep).join("/") | ||
| } | ||
|
|
||
| return str | ||
| } |
There was a problem hiding this comment.
Cross‑platform bug: Windows paths can normalize incorrectly on POSIX.
normalizePath uses platform isAbsolute/resolve/relative, so a Windows path like C:\app\dist on Linux is treated as relative and becomes ./C:\app\dist. Guard by detecting path “flavor” and using path.win32/path.posix accordingly; only normalize when the basePath flavor matches.
- private normalizePath(str: string): string {
+ private normalizePath(str: string): string {
if (!this.looksLikePath(str)) {
return str
}
- const absolutePath = isAbsolute(str) ? str : resolve(this.basePath, str)
-
- const relativePath = relative(this.basePath, absolutePath)
-
- if (relativePath && !relativePath.startsWith("..")) {
- return "./" + relativePath.split(sep).join("/")
- }
-
- return str
+ const isWin = this.isWindowsLike(str)
+ const baseIsWin = this.isWindowsLike(this.basePath)
+ // Don't attempt cross-flavor normalization
+ if (isWin !== baseIsWin) {
+ return str
+ }
+ const p = isWin ? require("path").win32 : require("path").posix
+ const absolutePath = p.isAbsolute(str) ? str : p.resolve(this.basePath, str)
+ const relativePath = p.relative(this.basePath, absolutePath)
+ if (relativePath && !relativePath.startsWith("..")) {
+ return "./" + relativePath.replace(/[\\]+/g, "/")
+ }
+ return str
}
+
+ private isWindowsLike(s: string): boolean {
+ return /^[A-Za-z]:[\\/]/.test(s) || s.startsWith("\\\\")
+ }| static detectBasePath(config: any): string | undefined { | ||
| const paths: string[] = [] | ||
|
|
||
| const extractPaths = (value: any): void => { | ||
| if (typeof value === "string" && isAbsolute(value)) { | ||
| paths.push(value) | ||
| } else if (Array.isArray(value)) { | ||
| value.forEach(extractPaths) | ||
| } else if (value && typeof value === "object") { | ||
| Object.values(value).forEach(extractPaths) | ||
| } | ||
| } | ||
|
|
||
| extractPaths(config) | ||
|
|
||
| if (paths.length === 0) { | ||
| return undefined | ||
| } | ||
|
|
||
| const commonPrefix = this.findCommonPrefix(paths) | ||
| return commonPrefix || undefined | ||
| } |
There was a problem hiding this comment.
detectBasePath misses Windows absolute paths on POSIX.
Use path.win32.isAbsolute in addition to the host platform check so Windows paths are recognized during detection.
- const extractPaths = (value: any): void => {
- if (typeof value === "string" && isAbsolute(value)) {
+ const { posix, win32 } = require("path")
+ const extractPaths = (value: any): void => {
+ if (
+ typeof value === "string" &&
+ (posix.isAbsolute(value) || win32.isAbsolute(value))
+ ) {
paths.push(value)
} else if (Array.isArray(value)) {
value.forEach(extractPaths)
} else if (value && typeof value === "object") {
Object.values(value).forEach(extractPaths)
}
}📝 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.
| static detectBasePath(config: any): string | undefined { | |
| const paths: string[] = [] | |
| const extractPaths = (value: any): void => { | |
| if (typeof value === "string" && isAbsolute(value)) { | |
| paths.push(value) | |
| } else if (Array.isArray(value)) { | |
| value.forEach(extractPaths) | |
| } else if (value && typeof value === "object") { | |
| Object.values(value).forEach(extractPaths) | |
| } | |
| } | |
| extractPaths(config) | |
| if (paths.length === 0) { | |
| return undefined | |
| } | |
| const commonPrefix = this.findCommonPrefix(paths) | |
| return commonPrefix || undefined | |
| } | |
| static detectBasePath(config: any): string | undefined { | |
| const paths: string[] = [] | |
| const { posix, win32 } = require("path") | |
| const extractPaths = (value: any): void => { | |
| if ( | |
| typeof value === "string" && | |
| (posix.isAbsolute(value) || win32.isAbsolute(value)) | |
| ) { | |
| paths.push(value) | |
| } else if (Array.isArray(value)) { | |
| value.forEach(extractPaths) | |
| } else if (value && typeof value === "object") { | |
| Object.values(value).forEach(extractPaths) | |
| } | |
| } | |
| extractPaths(config) | |
| if (paths.length === 0) { | |
| return undefined | |
| } | |
| const commonPrefix = this.findCommonPrefix(paths) | |
| return commonPrefix || undefined | |
| } |
🤖 Prompt for AI Agents
In package/configDiffer/pathNormalizer.ts around lines 121 to 142,
detectBasePath currently only uses the host-platform isAbsolute check and
therefore misses Windows-style absolute paths when running on POSIX; update the
path detection to check both platform-specific and Windows absolute paths (e.g.,
use path.win32.isAbsolute(value) in addition to the existing isAbsolute(value)),
applying this combined check inside extractPaths for string values so Windows
paths (drive letters and backslash forms) are collected and included when
computing the common prefix.
| const splitPaths = paths.map((p) => p.split(sep)) | ||
| let prefix: string[] = [] | ||
|
|
||
| for (let i = 0; i < splitPaths[0].length; i++) { | ||
| const segment = splitPaths[0][i] | ||
| if (splitPaths.every((p) => p[i] === segment)) { | ||
| prefix.push(segment) | ||
| } else { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return prefix.join(sep) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Common-prefix computation is platform-bound; fails with backslashes.
Split on both separators and rejoin with the appropriate one based on inputs.
- const splitPaths = paths.map((p) => p.split(sep))
+ const splitPaths = paths.map((p) => p.split(/[\\/]+/))
let prefix: string[] = []
@@
- return prefix.join(sep)
+ const useWinSep = paths.every((p) => /^[A-Za-z]:[\\/]/.test(p) || p.startsWith("\\\\"))
+ return prefix.join(useWinSep ? "\\" : "/")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const splitPaths = paths.map((p) => p.split(sep)) | |
| let prefix: string[] = [] | |
| for (let i = 0; i < splitPaths[0].length; i++) { | |
| const segment = splitPaths[0][i] | |
| if (splitPaths.every((p) => p[i] === segment)) { | |
| prefix.push(segment) | |
| } else { | |
| break | |
| } | |
| } | |
| return prefix.join(sep) | |
| } | |
| const splitPaths = paths.map((p) => p.split(/[\\/]+/)) | |
| let prefix: string[] = [] | |
| for (let i = 0; i < splitPaths[0].length; i++) { | |
| const segment = splitPaths[0][i] | |
| if (splitPaths.every((p) => p[i] === segment)) { | |
| prefix.push(segment) | |
| } else { | |
| break | |
| } | |
| } | |
| const useWinSep = paths.every((p) => /^[A-Za-z]:[\\/]/.test(p) || p.startsWith("\\\\")) | |
| return prefix.join(useWinSep ? "\\" : "/") |
🤖 Prompt for AI Agents
In package/configDiffer/pathNormalizer.ts around lines 153 to 166, the
common-prefix logic only splits on path.sep and thus fails when inputs use
backslashes; instead normalize by splitting each input on both separators (e.g.
split on /|\\), determine which separator to use for the output by inspecting
the original inputs (if any path contains '\' use backslash, otherwise use '/'),
compute the common path segments across the normalized segment arrays, and
return those segments joined with the chosen separator; also handle empty inputs
or single-item arrays gracefully.
a2e4bdd to
92e13c5
Compare
Implements Phase 1 of issue #667: Core Diff Engine Features: - Deep object diff algorithm with path-level granularity - Support for nested objects, arrays, and special types (functions, RegExp, Date) - Path normalization utilities for comparing absolute paths - Multiple output formats: detailed, summary, JSON, YAML - Advanced filtering options: ignore keys, ignore paths (with wildcards) - Comprehensive test coverage with 39 passing tests New CLI command: - bin/diff-bundler-config: Compare two config files - Supports JSON, YAML, JS, and TS config files - Exit code 0 if no differences, 1 if differences found Documentation: - Complete diff tool guide in docs/config-diff.md - Updated README.md with diff examples - Usage examples and tips 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add configDiffer to package.json exports for proper module resolution - Add comprehensive 'Why use this?' section explaining advantages over visual diff - Document 5 common troubleshooting workflows with examples - Explain integration with --doctor mode - Add practical guidance on what to look for in different scenarios 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ation
Address code review feedback:
1. Fix basePath detection to use separate paths for left/right configs
- Each side now detects its own base path independently
- More accurate normalization when comparing configs from different locations
2. Remove no-op map in createHumanPath
- Simplified path formatting by removing pointless transformation
- Cleaner, more maintainable code
3. Fix looksLikePath to avoid normalizing URLs and module specifiers
- Explicitly exclude URL schemes (http://, webpack://, etc.)
- Exclude npm module specifiers starting with @
- Only match actual filesystem paths (absolute, relative, home, Windows drives)
- Prevents incorrect normalization of webpack:// URLs and @scope/package names
4. Replace summary/detailed formats with contextual format
- Shows "What it does" explanations for webpack config keys
- Includes impact analysis (e.g., "Enabling production optimizations")
- Links to official webpack documentation
- Displays default values where applicable
- Much more useful for understanding WHY something changed
5. Add comprehensive configuration documentation database
- 20+ documented webpack/rspack configuration keys
- Covers mode, devtool, optimization, output, devServer, etc.
- Extensible for future additions
New output format example:
1. [~] mode
What it does:
Defines the environment mode (development, production, or none).
Controls built-in optimizations and defaults.
Affects: Minification, tree-shaking, source maps, and performance optimizations
Old value: "development"
New value: "production"
Impact: Enabling production optimizations (minification, tree-shaking)
Documentation: https://webpack.js.org/configuration/mode/
Summary format is now concise: "8 changes: +3 -1 ~4"
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Changes based on feedback:
1. Remove redundant value type display
- Type information doesn't add value to the diff output
- Users can see the type from the value itself
2. Replace "old/new" with file-based labels
- Extract meaningful short names from filenames
- webpack-development-client.yaml -> "dev-client"
- webpack-production-server.yaml -> "prod-server"
- Falls back to "left"/"right" if no pattern matches
3. Show values in a clearer format:
Before:
Old value: "development"
New value: "production"
After:
Values:
dev-client: "development"
prod-client: "production"
4. Design supports future N-way comparison
- Format naturally extends to comparing 3+ configs
- Could show dev-client, dev-server, prod-client, prod-server all in one view
- Labels make it clear which value comes from which file
Example output:
3. [~] mode
What it does:
Defines the environment mode (development, production, or none).
Affects: Minification, tree-shaking, source maps
Values:
dev: "development"
prod: "production"
Impact: Enabling production optimizations
Much clearer which config has which value!
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
config-diff-engine
branch
from
92e13c5 to
50c1935
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
package/configDiffer/diffEngine.ts (3)
130-143: Don’t treat falsy new values as “missing”.The
newValue || oldValuefallback on Line 142 misreports the type for legitimate falsy values (e.g., 0 → “number” becomes “string” ifoldValuewas"0"). Use an undefined check instead.- valueType: valueType || this.getValueType(newValue || oldValue) + valueType: + valueType ?? + this.getValueType(newValue !== undefined ? newValue : oldValue)
163-175: Wildcard ignore logic breaks on array paths and custom separators.The current expansion (Line 166) leaves
[ ]and other metacharacters unescaped, so a pattern likemodule.rules[*]never matches array entries. Additionally, the prefix check hardcodes".", ignoring--path-separator. Escape all regex metacharacters and honor the configured separator.- const humanPath = this.createHumanPath(path) - return this.options.ignorePaths.some((ignorePath) => { - if (ignorePath.includes("*")) { - const escapedPattern = ignorePath - .replace(/\./g, "\\.") - .replace(/\*/g, ".*") - const pattern = new RegExp("^" + escapedPattern + "$") - return pattern.test(humanPath) - } - return humanPath === ignorePath || humanPath.startsWith(ignorePath + ".") - }) + const humanPath = this.createHumanPath(path) + const separator = this.options.pathSeparator + return this.options.ignorePaths.some((ignorePath) => { + if (ignorePath.includes("*")) { + const escapedSegments = ignorePath + .split("*") + .map((segment) => segment.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")) + const pattern = new RegExp("^" + escapedSegments.join(".*") + "$") + return pattern.test(humanPath) + } + return ( + humanPath === ignorePath || + humanPath.startsWith(ignorePath + separator) + ) + })
51-78: Respect maxDepth without hiding real differences.The guard on Line 51 returns immediately, so with
--max-depth=0every nested change disappears and the diff falsely reports “no differences”. Emit an aggregated entry before returning.- if (this.options.maxDepth !== null && depth > this.options.maxDepth) { - return - } - - const leftType = this.getValueType(left) - const rightType = this.getValueType(right) + const leftType = this.getValueType(left) + const rightType = this.getValueType(right) + + if (this.options.maxDepth !== null && depth > this.options.maxDepth) { + if (left === undefined && right !== undefined) { + this.addEntry("added", path, undefined, right, rightType) + } else if (right === undefined && left !== undefined) { + this.addEntry("removed", path, left, undefined, leftType) + } else if (!this.areEqual(left, right)) { + this.addEntry("changed", path, left, right, leftType) + } else if (this.options.includeUnchanged) { + this.addEntry("unchanged", path, left, right, leftType) + } + return + }package/configDiffer/formatter.ts (1)
246-249: Matchoutput.filenameprecisely (respecting custom separators).Line 246 matches any path containing the substring, so a key like
module.rules[0].output.filenameTemplatetriggers the cache-busting note, and using a non-dot separator stops matching altogether. Compare against exact segment names.- if (path.includes("output.filename")) { + if ( + path === "output.filename" || + path.startsWith("output.filename" + ".") + ) { if (String(newVal).includes("[contenthash]")) { return "Cache busting enabled - better long-term caching" } }If you add custom
pathSeparatorsupport here too, reuse the same separator the formatter already exposes.package/configDiffer/pathNormalizer.ts (3)
124-141: Detect absolute paths from both POSIX and Windows inputs.Line 125 only checks
path.isAbsolute, soC:\project\distnever seedsdetectBasePathon POSIX. Use bothposixPath.isAbsoluteandwin32Path.isAbsoluteso cross-platform exports share the same base path.- if (typeof value === "string" && isAbsolute(value)) { + if ( + typeof value === "string" && + (posixPath.isAbsolute(value) || win32Path.isAbsolute(value)) + ) { paths.push(value)
149-166: Handle mixed separators when computing the common prefix.
split(sep)fails for Windows-style strings while running on POSIX (Line 153), so the common prefix collapses to the whole path and normalization regresses. Split on both separators and choose the appropriate joiner.- const splitPaths = paths.map((p) => p.split(sep)) + const splitPaths = paths.map((p) => p.split(/[\\/]+/)) @@ - return prefix.join(sep) + const useWinSeparator = paths.some((p) => p.includes("\\")) + return prefix.join(useWinSeparator ? "\\" : "/")
41-55: Stop mangling Windows paths when normalizing on POSIX.On Line 46 we feed any string with backslashes into the host
path.resolve/relative. Running this on macOS/Linux withC:\build\distrewrites the value to something like./C:\build\dist, so the diff output lies about the original path. Detect the path “flavor”, bail on cross-flavor cases, and usepath.win32/path.posixhelpers accordingly.-import { resolve, isAbsolute, relative, sep } from "path" +import { sep, posix as posixPath, win32 as win32Path } from "path" @@ private normalizePath(str: string): string { if (!this.looksLikePath(str)) { return str } - const absolutePath = isAbsolute(str) ? str : resolve(this.basePath, str) - - const relativePath = relative(this.basePath, absolutePath) + const pathModule = this.isWindowsLike(str) ? win32Path : posixPath + const baseIsWindows = this.isWindowsLike(this.basePath) + if (baseIsWindows !== this.isWindowsLike(str)) { + return str + } + + const absolutePath = pathModule.isAbsolute(str) + ? str + : pathModule.resolve(this.basePath, str) + const relativePath = pathModule.relative(this.basePath, absolutePath) if (relativePath && !relativePath.startsWith("..")) { - return "./" + relativePath.split(sep).join("/") + return "./" + relativePath.replace(/[\\]+/g, "/") } return str } + + private isWindowsLike(value: string): boolean { + return /^[A-Za-z]:[\\/]/.test(value) || value.startsWith("\\\\") + }package/configDiffer/cli.ts (2)
76-82: Dynamic require('fs') is redundant.Line 77 uses
require("fs")despitefsbeing imported via named imports at the top of the file (line 1:import { existsSync, readFileSync } from "fs"). This creates inconsistency and was flagged in a previous review but remains unaddressed.Import
writeFileSyncat the top and use it directly:-import { existsSync, readFileSync } from "fs" +import { existsSync, readFileSync, writeFileSync } from "fs" import { resolve, extname } from "path" import { load as loadYaml } from "js-yaml"Then update the file writing code:
if (options.output) { - const fs = require("fs") - fs.writeFileSync(options.output, output, "utf8") + writeFileSync(options.output, output, "utf8") console.log(`Diff written to: ${options.output}`) } else {
183-190: Require cache clearing limitation remains undocumented.Line 183 clears the require cache, but this only works for CommonJS modules. For ES modules, configs won't reload properly in the same process. This was flagged in a previous review but the limitation is still not documented.
Add a comment documenting this limitation:
delete require.cache[resolvedPath] + // Note: Cache clearing only works for CommonJS modules. + // For ES modules, configs may not reload properly in the same process. let loaded = require(resolvedPath) if (typeof loaded === "object" && "default" in loaded) { loaded = loaded.default }
🧹 Nitpick comments (4)
docs/config-diff.md (2)
9-38: Promote emphasis blocks to real headings.Markdownlint (MD036) flags the repeated
**Heading**pattern starting around Line 9. Turning these into level-3 headings (e.g.,### Configuration-Specific Intelligence) keeps the styling and satisfies the linter.
329-351: Add a language tag to the command reference fence.Markdownlint (MD040) complains because the block beginning near Line 330 lacks a language. Add
textorbashafter the opening ``` to clear the warning.test/configDiffer/formatter.test.js (1)
4-37: Consider removing unusedvalueTypefield from mock data.The
valueTypefield appears on lines 16, 22, and 29 but doesn't seem to be used by the DiffFormatter based on the provided formatter.ts code. This might be legacy data or an implementation detail that's no longer surfaced in the output.If valueType is truly unused, apply this diff:
{ operation: "added", path: { path: ["newKey"], humanPath: "newKey" }, - newValue: "newValue", - valueType: "string" + newValue: "newValue" }, { operation: "removed", path: { path: ["oldKey"], humanPath: "oldKey" }, - oldValue: "oldValue", - valueType: "string" + oldValue: "oldValue" }, { operation: "changed", path: { path: ["changedKey"], humanPath: "changedKey" }, oldValue: "oldValue", - newValue: "newValue", - valueType: "string" + newValue: "newValue" }package/configDiffer/cli.ts (1)
114-151: Unknown CLI arguments are silently ignored.The argument parser doesn't validate or warn about unrecognized arguments. If a user mistypes
--formt=jsoninstead of--format=json, the tool will silently use the default format, which could cause confusion.Consider tracking and warning about unknown arguments:
const parseValue = (arg: string, prefix: string): string => { const value = arg.substring(prefix.length) if (value.length === 0) { throw new Error(`${prefix} requires a value`) } return value } + const unknownArgs: string[] = [] + for (const arg of args) { if (arg === "--help" || arg === "-h") { options.help = true } else if (arg.startsWith("--left=")) { // ... existing cases ... } else if (arg.startsWith("--path-separator=")) { options.pathSeparator = parseValue(arg, "--path-separator=") + } else if (arg.startsWith("--")) { + unknownArgs.push(arg) } } + if (unknownArgs.length > 0 && !options.help) { + console.warn(`Warning: Unknown arguments ignored: ${unknownArgs.join(", ")}`) + } + return options }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
README.md(1 hunks)bin/diff-bundler-config(1 hunks)docs/config-diff.md(1 hunks)lib/install/bin/diff-bundler-config(1 hunks)package.json(1 hunks)package/configDiffer/cli.ts(1 hunks)package/configDiffer/configDocs.ts(1 hunks)package/configDiffer/diffEngine.ts(1 hunks)package/configDiffer/formatter.ts(1 hunks)package/configDiffer/index.ts(1 hunks)package/configDiffer/pathNormalizer.ts(1 hunks)package/configDiffer/types.ts(1 hunks)test/configDiffer/diffEngine.test.js(1 hunks)test/configDiffer/formatter.test.js(1 hunks)test/configDiffer/pathNormalizer.test.js(1 hunks)tmp/examples/dev-config.yaml(1 hunks)tmp/examples/prod-config.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- lib/install/bin/diff-bundler-config
- test/configDiffer/pathNormalizer.test.js
- package/configDiffer/configDocs.ts
- package/configDiffer/types.ts
- package.json
- test/configDiffer/diffEngine.test.js
- package/configDiffer/index.ts
- tmp/examples/prod-config.yaml
- README.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: shakacode/shakapacker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-19T00:36:13.668Z
Learning: Applies to lib/shakapacker/**/*.rb : When modifying core Shakapacker functionality, test changes with both webpack and rspack bundlers
📚 Learning: 2025-10-19T00:36:13.668Z
Learnt from: CR
Repo: shakacode/shakapacker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-19T00:36:13.668Z
Learning: Applies to lib/shakapacker/**/*.rb : When modifying core Shakapacker functionality, test changes with both webpack and rspack bundlers
Applied to files:
tmp/examples/dev-config.yamlbin/diff-bundler-configdocs/config-diff.md
🧬 Code graph analysis (5)
test/configDiffer/formatter.test.js (2)
package/configDiffer/formatter.ts (1)
DiffFormatter(5-289)package/configDiffer/index.ts (1)
DiffFormatter(3-3)
package/configDiffer/pathNormalizer.ts (2)
package/configDiffer/index.ts (2)
PathNormalizer(4-4)NormalizedConfig(11-11)package/configDiffer/types.ts (1)
NormalizedConfig(44-48)
package/configDiffer/cli.ts (4)
package/configDiffer/types.ts (1)
DiffOptions(34-42)package/configDiffer/pathNormalizer.ts (1)
PathNormalizer(4-167)package/configDiffer/diffEngine.ts (1)
DiffEngine(9-299)package/configDiffer/formatter.ts (1)
DiffFormatter(5-289)
package/configDiffer/diffEngine.ts (1)
package/configDiffer/types.ts (5)
DiffOptions(34-42)DiffEntry(8-14)DiffResult(16-32)DiffOperation(1-1)DiffPath(3-6)
package/configDiffer/formatter.ts (2)
package/configDiffer/types.ts (3)
DiffResult(16-32)DiffEntry(8-14)DiffOperation(1-1)package/configDiffer/configDocs.ts (1)
getDocForKey(161-163)
🪛 markdownlint-cli2 (0.18.1)
docs/config-diff.md
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
15-15: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
27-27: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
34-34: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
329-329: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: claude-review
🔇 Additional comments (6)
tmp/examples/dev-config.yaml (2)
14-15: Clarify if simplified plugin representation is intentional.The
pluginsarray (lines 14–15) lists plugins as strings rather than instantiated objects, which deviates from standard webpack configuration syntax. While this may be intentional for a simplified example file, please confirm:
- Is this meant to be a schematic/documentation example rather than an executable config?
- Should plugins be shown as fully instantiated objects (e.g.,
{ name: HtmlWebpackPlugin, options: {...} }) for more realistic representation?This matters for validating that users can rely on this as a reference when comparing real configurations.
1-18: Example file looks good overall.The configuration provides a clear, minimal example of webpack development settings (mode, devtool, output, cache, optimization, plugins, devServer) and is well-suited for demonstration and testing of the diff engine. The YAML syntax is valid.
If this is intended as a reference template for users, consider either: (1) making a note in accompanying docs that it's a simplified schema, or (2) expanding plugins to show realistic object structure. For internal testing/fixtures, the current form is acceptable.
bin/diff-bundler-config (1)
1-11: LGTM.The shim cleanly delegates to the shared CLI handler and exits with the provided code.
test/configDiffer/formatter.test.js (1)
39-129: Tests provide adequate basic coverage.The test suite validates the four main formatter outputs (JSON, YAML, summary, detailed) with appropriate assertions for structure and content presence. The tests are well-organized and sufficient for smoke-testing the formatter's output formats.
package/configDiffer/cli.ts (2)
38-49: Per-side base path detection correctly implemented.The code now detects base paths independently for left and right configs (lines 39-42) and creates separate normalizers (lines 44-45), addressing the previous review concern about cross-machine path normalization.
16-89: CLI structure and implementation are well-designed.The overall CLI implementation is clean and well-organized:
- Clear separation of concerns (run, parseArguments, loadConfigFile, showHelp)
- Comprehensive help text with examples
- Proper error handling and exit codes
- Support for multiple input formats (JSON, YAML, JS, TS)
- Good integration with DiffEngine, DiffFormatter, and PathNormalizer
Also applies to: 91-196, 198-260
Summary
Implements Phase 1 of #667: Core Diff Engine for comparing webpack/rspack configurations.
This PR adds a comprehensive configuration comparison tool with contextual information that helps developers understand not just WHAT changed, but WHY it matters.
Example Output
Contextual Format (default)
Shows what each change means with documentation and impact analysis:
Summary Format
Concise one-liner for CI or quick checks:
JSON Format (for CI/CD)
Machine-readable output for automated processing:
{ "summary": { "totalChanges": 8, "added": 3, "removed": 1, "changed": 4 }, "entries": [ { "operation": "changed", "path": { "path": ["mode"], "humanPath": "mode" }, "oldValue": "development", "newValue": "production", "valueType": "string" } ] }Why Use This Instead of Visual Diff Tools?
Configuration-Specific Intelligence
Path Normalization
/Users/you/appand/home/ci/appboth become./appFocused Analysis
--ignore-pathsand--ignore-keys--ignore-paths="plugins.*"to focus on core configMultiple Output Formats
Semantic Understanding
Integration with --doctor Mode
The
--doctormode exports all configs in one command, optimized for comparison:Benefits:
Common Troubleshooting Workflows
"Works in Dev, Fails in Prod"
The contextual output will explain each difference and its impact.
After Upgrading Dependencies
See exactly what changed with documentation links to understand new defaults.
Webpack → Rspack Migration
bin/export-bundler-config --bundler=webpack --save --output=webpack.yaml rake shakapacker:switch_bundler bin/export-bundler-config --bundler=rspack --save --output=rspack.yaml bin/diff-bundler-config \ --left=webpack.yaml \ --right=rspack.yaml \ --ignore-paths="plugins.*"Focus on core config differences with contextual explanations.
Key Features
Core Diff Engine
Configuration Documentation
Path Normalization
webpack://,http://) and module specifiers (@scope/package)--no-normalize-pathsMultiple Output Formats
8 changes: +3 -1 ~4)Advanced Options
CLI Command
Supported File Formats
.json).yaml,.yml).js).ts) - requires ts-nodeTest Coverage
Comprehensive test suite with 39 passing tests covering:
Documentation
docs/config-diff.mdwith 5 troubleshooting workflowsCode Quality
All review feedback addressed:
Testing
All tests pass:
Next Steps
This implements Phase 1 of the roadmap in #667. Future phases will add:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores