Add YAML configuration file support for bin/export-bundler-config #686
Conversation
WalkthroughAdds YAML-driven configuration support and CLI commands for managing/exporting bundler build configs: introduces ConfigFileLoader and sample config generation, new CLI flags ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI (configExporter/cli.ts)
participant Loader as ConfigFileLoader
participant FS as File System
participant Bundler as Bundler (webpack/rspack)
participant Writer as fileWriter
User->>CLI: run export-bundler-config [--init | --list-builds | --build NAME | legacy]
alt --init
CLI->>Loader: generateSampleConfigFile()
CLI->>FS: write .bundler-config.yml (sample)
CLI-->>User: show next steps
else --list-builds
CLI->>Loader: listBuilds()
Loader->>FS: read .bundler-config.yml
Loader-->>CLI: builds summary
CLI-->>User: print builds
else --build NAME
CLI->>Loader: resolveBuild(NAME, options, defaultBundler)
Loader->>FS: read & parse YAML
Loader->>Loader: resolve bundler, expand env, map bundler_env→args
Loader-->>CLI: ResolvedBuildConfig
CLI->>Bundler: invoke with resolved args/configFile
Bundler-->>CLI: produce configs
CLI->>Writer: generateFilename(bundler, env, type, format, buildName)
CLI->>FS: write outputs
CLI-->>User: export results
else legacy
CLI->>Bundler: invoke via legacy flags
Bundler-->>CLI: configs
CLI->>Writer: generateFilename(bundler, env, type, format)
CLI->>FS: write outputs
CLI-->>User: export results
end
note over Loader,CLI: Bundler resolution precedence: CLI flag > build.bundler > config.default_bundler > fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review: YAML Configuration SupportThis is a comprehensive and well-thought-out feature addition. Overall, the implementation is solid, but I've identified several important issues. Critical Issues1. Missing Trailing Newlines 2. Missing Test Coverage 3. Security: Unvalidated YAML Loading Important Issues4. Error Handling Type Safety 5. Environment Variable Expansion Security 6. Outputs Array Validation 7. File Naming Special Characters Suggestions
What's Good
Before Merging
OverallWell-architected feature with good design. Needs test coverage and security fixes before merging. Request changes for critical issues. Great work on this enhancement! |
Code Review for PR #686Strengths
Critical Issues
Medium Priority
Test Coverage - MAJOR CONCERNNo test files included. Need tests for:
Security
Code Quality
Action Items
Overall AssessmentValuable feature with solid design BUT has critical issues (incomplete files, type mismatches) and missing tests. Recommendation: Request changes - Fix critical issues and add tests before merging. Great work! Looking forward to the completed implementation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
bundler-config-enhancement-spec.md (2)
191-197: File extension mismatch with implementationExamples use .yml, but the generator produces .yaml (see FileWriter.generateFilename). Align examples to .yaml or change generator to .yml to avoid confusion.
216-238: Add language to fenced code block (markdownlint MD040)The “Output:” block lacks a language. Use a neutral language like “text”.
Apply:
-``` +```text Available builds in .bundler-config.yml: ...</blockquote></details> <details> <summary>package/configExporter/types.ts (1)</summary><blockquote> `55-56`: **Constrain outputs to known values** Limit outputs to "client" | "server" | "all" to catch typos at compile time and match downstream usage. ```diff - outputs?: string[] + outputs?: Array<"client" | "server" | "all">package/configExporter/cli.ts (2)
660-663: Reduce unsolicited console noiseAuto-detected bundler is logged unconditionally. Consider gating behind --verbose to keep stdout clean for scripting/inspection.
- console.log(`[Config Exporter] Auto-detected bundler: ${bundler}`) + if (process.env.VERBOSE) { + console.log(`[Config Exporter] Auto-detected bundler: ${bundler}`) + }
371-390: Handle computed bundlerEnvArgs in the CLI
resolvedBuild.bundlerEnvArgs is generated by ConfigFileLoader.resolveBuild but never used in cli.ts. Either pass bundlerEnvArgs when invoking function-style configs or include it in the CLI output metadata, or remove bundlerEnvArgs from BuildConfig to eliminate dead code.package/configExporter/configFile.ts (3)
31-41: Improve YAML error diagnosticsPass filename to js-yaml.load for clearer parse errors.
- const parsed = loadYaml(content) as BundlerConfigFile + const parsed = loadYaml(content, { filename: this.configFilePath }) as BundlerConfigFile[retrieved_learnings]
81-92: Validate outputs tokensCurrently only checks array shape. Validate elements to be "client" | "server" | "all" to prevent downstream casts of invalid values.
- if (build.outputs && !Array.isArray(build.outputs)) { + if (build.outputs && !Array.isArray(build.outputs)) { throw new Error( `Invalid outputs in build '${name}'. Must be an array of strings.` ) - } + } + if (Array.isArray(build.outputs)) { + const allowed = new Set(["client", "server", "all"]) + const invalid = build.outputs.filter((o) => !allowed.has(String(o))) + if (invalid.length > 0) { + throw new Error( + `Invalid outputs in build '${name}': ${invalid.join( + ", " + )}. Allowed: client, server, all.` + ) + } + }
210-229: List output: clarify default bundler labelExpression may print a literal “webpack (default)”. Consider formatting to distinguish when default is used.
- const bundler = - build.bundler || config.default_bundler || "webpack (default)" + const bundler = + build.bundler ?? + config.default_bundler ?? + "webpack" + const bundlerLabel = + build.bundler ? bundler : config.default_bundler ? `${bundler} (from default_bundler)` : `${bundler} (fallback)` ... - console.log(` Bundler: ${bundler}`) + console.log(` Bundler: ${bundlerLabel}`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bundler-config-enhancement-spec.md(1 hunks)package/configExporter/cli.ts(13 hunks)package/configExporter/configFile.ts(1 hunks)package/configExporter/fileWriter.ts(1 hunks)package/configExporter/index.ts(1 hunks)package/configExporter/types.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
package/configExporter/fileWriter.ts (1)
lib/shakapacker/configuration.rb (1)
bundler(108-110)
package/configExporter/configFile.ts (1)
package/configExporter/types.ts (3)
BundlerConfigFile(45-48)ExportOptions(1-20)ResolvedBuildConfig(59-67)
package/configExporter/cli.ts (2)
package/configExporter/configFile.ts (2)
ConfigFileLoader(11-230)generateSampleConfigFile(232-393)package/configExporter/types.ts (1)
ExportOptions(1-20)
package/configExporter/types.ts (1)
package/configExporter/index.ts (3)
BundlerConfigFile(6-6)BuildConfig(7-7)ResolvedBuildConfig(8-8)
🪛 markdownlint-cli2 (0.18.1)
bundler-config-enhancement-spec.md
216-216: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
package/configExporter/index.ts (1)
2-9: Public exports LGTMType and symbol re-exports align with new config-file workflow. No issues.
Also applies to: 13-13
| } else if (arg === "--init") { | ||
| options.init = true | ||
| } else if (arg.startsWith("--config-file=")) { | ||
| options.configFile = parseValue(arg, "--config-file=") | ||
| } else if (arg.startsWith("--build=")) { | ||
| options.build = parseValue(arg, "--build=") | ||
| } else if (arg === "--list-builds") { | ||
| options.listBuilds = true | ||
| } |
There was a problem hiding this comment.
Missing --annotate flag handling (compatibility regression)
PR claims backward compatibility incl. --annotate, but only --no-annotate is parsed. Add support for --annotate.
} else if (arg === "--no-annotate") {
options.annotate = false
+ } else if (arg === "--annotate") {
+ options.annotate = true
} else if (arg === "--verbose") {
options.verbose = trueCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In package/configExporter/cli.ts around lines 139 to 147, the CLI parsing only
recognizes --no-annotate but not the legacy --annotate flag, causing a
compatibility regression; update the argument parsing to detect "--annotate" and
set options.annotate = true when present (and keep existing handling for
"--no-annotate" to set it false), ensuring you parse "--annotate" before any
generic startsWith checks or add an explicit else-if for "--annotate" alongside
the other literal flag checks so both forms are supported.
| private expandEnvironmentVariables( | ||
| vars: Record<string, string>, | ||
| bundler: string | ||
| ): Record<string, string> { | ||
| const expanded: Record<string, string> = {} | ||
|
|
||
| for (const [key, value] of Object.entries(vars)) { | ||
| expanded[key] = this.expandString(value, bundler) | ||
| } | ||
|
|
||
| return expanded | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Coerce env values to strings before expansion
YAML scalars may be non-strings; calling replace on numbers/booleans will throw. Coerce to string first.
- for (const [key, value] of Object.entries(vars)) {
- expanded[key] = this.expandString(value, bundler)
+ for (const [key, value] of Object.entries(vars)) {
+ expanded[key] = this.expandString(String(value), bundler)
}📝 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 expandEnvironmentVariables( | |
| vars: Record<string, string>, | |
| bundler: string | |
| ): Record<string, string> { | |
| const expanded: Record<string, string> = {} | |
| for (const [key, value] of Object.entries(vars)) { | |
| expanded[key] = this.expandString(value, bundler) | |
| } | |
| return expanded | |
| } | |
| private expandEnvironmentVariables( | |
| vars: Record<string, string>, | |
| bundler: string | |
| ): Record<string, string> { | |
| const expanded: Record<string, string> = {} | |
| for (const [key, value] of Object.entries(vars)) { | |
| expanded[key] = this.expandString(String(value), bundler) | |
| } | |
| return expanded | |
| } |
🤖 Prompt for AI Agents
In package/configExporter/configFile.ts around lines 158 to 169, the loop
assumes env values are strings and calls string methods during expansion, which
breaks for non-string YAML scalars; coerce each value to a string before calling
expandString (e.g., use String(value) or value?.toString()) so expandString
always receives a string, and assign the result to expanded[key]; handle
null/undefined by explicitly converting them to an empty string if desired.
Code Review - PR #686: YAML Configuration File SupportThank you for this comprehensive enhancement! This is a well-thought-out feature that adds significant value. Here's my detailed review: ✅ StrengthsCode Quality
Design
🔍 Issues & Recommendations1. Security Concerns
|
Code Review - PR #686: Add YAML Configuration File SupportThank you for this comprehensive feature addition! I've reviewed the code quality, architecture, potential issues, security, performance, and test coverage. Here's my detailed feedback: ✅ StrengthsArchitecture & Design
Code Quality
🔴 Critical Issues1. Missing Type Definitions (High Priority)The // Missing from types.ts:
export interface ExportOptions {
// ... existing fields ...
init?: boolean // ❌ MISSING
configFile?: string // ❌ MISSING
build?: string // ❌ MISSING
listBuilds?: boolean // ❌ MISSING
}
export interface ConfigMetadata {
// ... existing fields ...
buildName?: string // ❌ MISSING
}
// These interfaces are completely missing:
export interface BundlerConfigFile { ... }
export interface BuildConfig { ... }
export interface ResolvedBuildConfig { ... }Impact: This will cause TypeScript compilation errors. Fix: Update 2. Incomplete
|
Code Review: YAML Configuration SupportThis is a well-designed feature, but there are critical issues that need to be addressed: Critical Issues1. Missing File Content: The configFile.ts is truncated in the diff (150/393 lines shown) 2. No Test Coverage: Zero tests for the new functionality. Required tests include:
3. Runtime Error - Parameter Mismatch:
4. Missing Type Definitions:
Code Quality Issues5. Error Handling: Creates ConfigFileLoader twice for existence check Security
What's Done Well✅ Excellent backward compatibility Pre-Merge Checklist
This will be a great feature once these issues are resolved! |
Code Review: YAML Configuration Support for bin/export-bundler-configOverall, this is a well-structured and thoughtful PR that adds valuable functionality. The implementation is comprehensive, backward-compatible, and well-documented. However, there are several areas that need attention before merging. 🔴 Critical Issues1. Missing Test Coverage (BLOCKER)The PR adds 1,125+ lines of new code across multiple TypeScript files but includes zero automated tests. This is a significant risk. Required:
Suggested test file locations:
2. Missing Implementation FilesThe PR diff shows changes to Action needed: Ensure all new files are included in the PR:
3. Security: Path Traversal Validation IncompleteThe In configFile.ts (from diff): private validateConfigPath(): void {
const absPath = resolve(this.configFilePath)
const cwd = process.cwd()
const rel = relative(cwd, absPath)
if (rel.startsWith("..") || (isAbsolute(rel) && !absPath.startsWith(cwd))) {
throw new Error(...)
}
}Issues:
Recommendation: private validateConfigPath(): void {
const absPath = realpathSync(this.configFilePath) // Resolves symlinks
const cwd = realpathSync(process.cwd())
const rel = relative(cwd, absPath)
if (rel.startsWith('..') || isAbsolute(rel)) {
throw new Error(`Config file must be within project directory`)
}
}
|
Code Review - PR #686: YAML Configuration File SupportOverall AssessmentThis is a well-designed feature that adds significant value. However, several critical issues need addressing before merging. Critical Issues
Type Safety Issues
Strengths
RecommendationsBefore Merging:
Nice to Have:
Security & Performance
ConclusionValuable feature with solid design, but needs test coverage and bug fixes. TypeScript type mismatches and truncated file are blockers. Great work on the design! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
package/configExporter/configFile.ts (1)
207-218: Coerce env values to strings before expansion (prevents runtime TypeError)YAML scalars can be non-strings; calling replace on them will throw.
- for (const [key, value] of Object.entries(vars)) { - expanded[key] = this.expandString(value, bundler) - } + for (const [key, value] of Object.entries(vars)) { + expanded[key] = this.expandString(String(value ?? ""), bundler) + }
🧹 Nitpick comments (3)
package/configExporter/configFile.ts (3)
35-45: Harden path traversal check (simplify and avoid brittle startsWith)Use relative + isAbsolute only; drop the cwd prefix check which can be misleading on Windows and with missing separators.
private validateConfigPath(): void { - const absPath = resolve(this.configFilePath) - const cwd = process.cwd() + const absPath = resolve(this.configFilePath) + const cwd = resolve(process.cwd()) const rel = relative(cwd, absPath) - if (rel.startsWith("..") || (isAbsolute(rel) && !absPath.startsWith(cwd))) { + if (rel.startsWith("..") || isAbsolute(rel)) { throw new Error( `Config file must be within project directory. Attempted path: ${this.configFilePath}` ) } }
181-186: Validate per-build configFile path is within project (same policy as loader path)Prevent using a config outside the project (accidental or malicious).
- const configFile = build.config - ? this.expandEnvironmentVariables({ config: build.config }, bundler) - .config - : undefined + const configFile = build.config + ? this.expandEnvironmentVariables({ config: build.config }, bundler).config + : undefined + if (configFile) { + const abs = resolve(configFile) + const cwd = resolve(process.cwd()) + const rel = relative(cwd, abs) + if (rel.startsWith("..") || isAbsolute(rel)) { + throw new Error(`Per-build config must be within project directory. Got: ${configFile}`) + } + }
240-257: Support numeric bundler_env values (e.g., retries: 3 → --env retries=3)Coerce numbers to strings; optionally update the type to include number.
- private convertBundlerEnvToArgs( - bundlerEnv: Record<string, string | boolean> - ): string[] { + private convertBundlerEnvToArgs( + bundlerEnv: Record<string, string | boolean | number> + ): string[] { @@ - } else if (typeof value === "string") { + } else if (typeof value === "string" || typeof value === "number") { // String value becomes --env key=value args.push("--env", `${key}=${value}`) }If you adopt this, also widen the type in BuildConfig.bundler_env in package/configExporter/types.ts:
// in types.ts bundler_env?: Record<string, string | number | boolean>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package/configExporter/configFile.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
package/configExporter/configFile.ts (1)
package/configExporter/types.ts (3)
BundlerConfigFile(45-48)ExportOptions(1-20)ResolvedBuildConfig(59-67)
🔇 Additional comments (1)
package/configExporter/configFile.ts (1)
68-75: Clarify YAML duplicate-key behavior when using json:truejs-yaml v4’s json: true follows JSON semantics (allows duplicate mapping keys, last-one-wins). Default (json: false) throws on duplicates. Choose one:
Option A (strict, throw on warnings):
- const parsed = loadYaml(content, { - schema: FAILSAFE_SCHEMA, - json: true - }) as BundlerConfigFile + const parsed = loadYaml(content, { + schema: FAILSAFE_SCHEMA, + onWarning: (e) => { throw e } + }) as BundlerConfigFileOption B (accept last-key-wins): keep json:true and document this behavior in the spec.
Confirm which behavior you want.
| // Validate each build | ||
| for (const [name, build] of Object.entries(config.builds)) { | ||
| if ( | ||
| build.bundler && | ||
| build.bundler !== "webpack" && | ||
| build.bundler !== "rspack" | ||
| ) { | ||
| throw new Error( | ||
| `Invalid bundler '${build.bundler}' in build '${name}'. Must be 'webpack' or 'rspack'.` | ||
| ) | ||
| } | ||
|
|
||
| if (build.bundler_env && typeof build.bundler_env !== "object") { | ||
| throw new Error( | ||
| `Invalid bundler_env in build '${name}'. Must be an object.` | ||
| ) | ||
| } | ||
|
|
||
| if (build.environment && typeof build.environment !== "object") { | ||
| throw new Error( | ||
| `Invalid environment in build '${name}'. Must be an object.` | ||
| ) | ||
| } | ||
|
|
||
| if (build.outputs && !Array.isArray(build.outputs)) { | ||
| throw new Error( | ||
| `Invalid outputs in build '${name}'. Must be an array of strings.` | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Strengthen schema validation (null/array checks, element/value types, config type)
Avoid arrays/null for maps; ensure env values are strings; ensure bundler_env values are string|boolean; ensure outputs are string[]; ensure config is string.
// Validate each build
for (const [name, build] of Object.entries(config.builds)) {
@@
- if (build.bundler_env && typeof build.bundler_env !== "object") {
+ if (build.bundler_env) {
+ if (
+ typeof build.bundler_env !== "object" ||
+ build.bundler_env === null ||
+ Array.isArray(build.bundler_env)
+ ) {
+ throw new Error(
+ `Invalid bundler_env in build '${name}'. Must be an object.`
+ )
+ }
+ for (const [k, v] of Object.entries(build.bundler_env)) {
+ if (
+ !(typeof v === "string" || typeof v === "boolean")
+ ) {
+ throw new Error(
+ `Invalid bundler_env['${k}'] in build '${name}'. Must be string or boolean.`
+ )
+ }
+ }
- throw new Error(
- `Invalid bundler_env in build '${name}'. Must be an object.`
- )
}
@@
- if (build.environment && typeof build.environment !== "object") {
- throw new Error(
- `Invalid environment in build '${name}'. Must be an object.`
- )
- }
+ if (build.environment) {
+ if (
+ typeof build.environment !== "object" ||
+ build.environment === null ||
+ Array.isArray(build.environment)
+ ) {
+ throw new Error(
+ `Invalid environment in build '${name}'. Must be an object.`
+ )
+ }
+ for (const [k, v] of Object.entries(build.environment)) {
+ if (typeof v !== "string") {
+ throw new Error(
+ `Invalid environment['${k}'] in build '${name}'. Must be a string.`
+ )
+ }
+ }
+ }
@@
- if (build.outputs && !Array.isArray(build.outputs)) {
- throw new Error(
- `Invalid outputs in build '${name}'. Must be an array of strings.`
- )
- }
+ if (build.outputs) {
+ if (!Array.isArray(build.outputs) || !build.outputs.every((o) => typeof o === "string")) {
+ throw new Error(
+ `Invalid outputs in build '${name}'. Must be an array of strings.`
+ )
+ }
+ }
+ if (build.config && typeof build.config !== "string") {
+ throw new Error(
+ `Invalid config in build '${name}'. Must be a string path.`
+ )
+ }📝 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.
| // Validate each build | |
| for (const [name, build] of Object.entries(config.builds)) { | |
| if ( | |
| build.bundler && | |
| build.bundler !== "webpack" && | |
| build.bundler !== "rspack" | |
| ) { | |
| throw new Error( | |
| `Invalid bundler '${build.bundler}' in build '${name}'. Must be 'webpack' or 'rspack'.` | |
| ) | |
| } | |
| if (build.bundler_env && typeof build.bundler_env !== "object") { | |
| throw new Error( | |
| `Invalid bundler_env in build '${name}'. Must be an object.` | |
| ) | |
| } | |
| if (build.environment && typeof build.environment !== "object") { | |
| throw new Error( | |
| `Invalid environment in build '${name}'. Must be an object.` | |
| ) | |
| } | |
| if (build.outputs && !Array.isArray(build.outputs)) { | |
| throw new Error( | |
| `Invalid outputs in build '${name}'. Must be an array of strings.` | |
| ) | |
| } | |
| } | |
| // Validate each build | |
| for (const [name, build] of Object.entries(config.builds)) { | |
| if ( | |
| build.bundler && | |
| build.bundler !== "webpack" && | |
| build.bundler !== "rspack" | |
| ) { | |
| throw new Error( | |
| `Invalid bundler '${build.bundler}' in build '${name}'. Must be 'webpack' or 'rspack'.` | |
| ) | |
| } | |
| if (build.bundler_env) { | |
| if ( | |
| typeof build.bundler_env !== "object" || | |
| build.bundler_env === null || | |
| Array.isArray(build.bundler_env) | |
| ) { | |
| throw new Error( | |
| `Invalid bundler_env in build '${name}'. Must be an object.` | |
| ) | |
| } | |
| for (const [k, v] of Object.entries(build.bundler_env)) { | |
| if (!(typeof v === "string" || typeof v === "boolean")) { | |
| throw new Error( | |
| `Invalid bundler_env['${k}'] in build '${name}'. Must be string or boolean.` | |
| ) | |
| } | |
| } | |
| } | |
| if (build.environment) { | |
| if ( | |
| typeof build.environment !== "object" || | |
| build.environment === null || | |
| Array.isArray(build.environment) | |
| ) { | |
| throw new Error( | |
| `Invalid environment in build '${name}'. Must be an object.` | |
| ) | |
| } | |
| for (const [k, v] of Object.entries(build.environment)) { | |
| if (typeof v !== "string") { | |
| throw new Error( | |
| `Invalid environment['${k}'] in build '${name}'. Must be a string.` | |
| ) | |
| } | |
| } | |
| } | |
| if (build.outputs) { | |
| if ( | |
| !Array.isArray(build.outputs) || | |
| !build.outputs.every((o) => typeof o === "string") | |
| ) { | |
| throw new Error( | |
| `Invalid outputs in build '${name}'. Must be an array of strings.` | |
| ) | |
| } | |
| } | |
| if (build.config && typeof build.config !== "string") { | |
| throw new Error( | |
| `Invalid config in build '${name}'. Must be a string path.` | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In package/configExporter/configFile.ts around lines 104 to 133, the current
validation is too permissive: add checks that config.builds is an object (not
null/array), each build is an object, and reject null/array for maps; enforce
build.config is a string; enforce build.bundler_env is a plain object whose
values are only string or boolean; enforce build.environment is a plain object
whose values are only strings; enforce build.outputs, when present, is an array
and every element is a string; tighten bundler check remains the same. For each
failing condition throw an Error with the build name and offending key so
callers can pinpoint the exact invalid value.
Code Review - PR #686: YAML Configuration SupportThis PR adds YAML configuration file support to bin/export-bundler-config. The implementation is well-structured with good separation of concerns. Strengths
Critical Issues1. CRITICAL: Missing Test CoverageThe spec document shows testing tasks marked as TODO. This is a blocker. Add tests for:
2. Missing Trailing NewlinesPer CLAUDE.md requirements, all files MUST end with a trailing newline. 3. Code Quality Issues
4. Potential BugThe resolveBuild() method creates bundlerEnvArgs but I don't see where they're used. Please verify. 5. SecurityAdd warning in generated config about not committing secrets. RecommendationsBefore Merging:
Should Address:
AssessmentWell-architected code with good security practices. Main blocker is lack of test coverage. Should not be merged without tests. Status: Changes Requested Great work! 🚀 |
Code Review - PR #686: YAML Configuration SupportCompleted comprehensive review. This is a well-designed feature with excellent security practices and backward compatibility. Strengths
Critical Issues to Address1. Missing Trailing NewlinesPer CLAUDE.md, all files must end with trailing newline. Check: bundler-config-enhancement-spec.md, configFile.ts, types.ts, cli.ts, fileWriter.ts, index.ts 2. Missing Test Coverage (CRITICAL)Need comprehensive tests for:
3. Code Quality Issues
4. Missing Error Handling
Security Assessment: GOOD
Before Merge (Required):
Nice to Have:
ConclusionHigh-quality PR with thoughtful design. Main concerns: missing tests (critical), trailing newlines, error handling edge cases. Estimated effort: 4-6 hours for tests + 1 hour for fixes Great work on security and backward compatibility! |
This enhancement adds YAML configuration file support to bin/export-bundler-config,
allowing users to define and version control their build configurations.
Key Features:
- New --init command to generate sample .bundler-config.yml
- New --build=<name> flag to export specific build from config file
- New --list-builds command to show available builds
- New --webpack and --rspack flags for easy bundler switching
- Support for multiple build configurations with environment variables
- Environment variable expansion (${VAR}, ${VAR:-default}, ${BUNDLER})
- Automatic bundler_env to CLI args conversion
- Build-specific output naming (bundler-build-type.ext)
- Backward compatible with all existing CLI usage
File Structure:
- package/configExporter/configFile.ts - Config file loading and parsing
- package/configExporter/types.ts - Schema types for config file
- bundler-config-enhancement-spec.md - Full specification document
Usage Examples:
bin/export-bundler-config --init
bin/export-bundler-config --list-builds
bin/export-bundler-config --build=dev --save
bin/export-bundler-config --build=dev --save --rspack
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Security Improvements: - Use FAILSAFE_SCHEMA for YAML parsing to prevent code execution - Add path validation to prevent directory traversal attacks - Validate config file paths are within project directory Documentation: - Add comprehensive JSDoc comments to all public methods - Document parameters, return values, and error conditions - Add usage examples in class documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
justin808/bundler-config-yaml
branch
from
2a0801b to
bf8f93a
Compare
Code Review SummaryI've reviewed PR #686 adding YAML configuration file support for ✅ Strengths
🔴 Critical Issues1. Security: YAML Parsing Risk (configFile.ts:72-74)const parsed = loadYaml(content, {
schema: FAILSAFE_SCHEMA,
json: true
}) as BundlerConfigFileGood use of 2. Missing Input Validation (configFile.ts:244-264)The
3. Type Safety Issues (cli.ts:398-402)for (const [key, value] of Object.entries(resolvedBuild.environment)) {
process.env[key] = value
}No validation that Recommendation: Add runtime type checking: for (const [key, value] of Object.entries(resolvedBuild.environment)) {
if (typeof value !== 'string') {
throw new Error(`Environment variable ${key} must be a string, got ${typeof value}`)
}
process.env[key] = value
}
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
package/configExporter/cli.ts (2)
135-147: Add support for --annotate flag.Only
--no-annotateis parsed. The PR claims backward compatibility including--annotate, but this flag is not handled.Apply this diff:
} else if (arg === "--no-annotate") { options.annotate = false + } else if (arg === "--annotate") { + options.annotate = true } else if (arg === "--verbose") { options.verbose = true
396-402: Config's NODE_ENV is ignored due to default value.
options.envdefaults to"development"(line 63), so!options.envis always false and the config's NODE_ENV is never applied.Track whether
--envwas explicitly set during parsing:function parseArguments(args: string[]): ExportOptions { const options: ExportOptions = { bundler: undefined, - env: "development", + env: undefined, clientOnly: false,Then apply a default later:
function applyDefaults(options: ExportOptions): void { + if (options.env === undefined) { + options.env = "development" + } if (options.doctor) {package/configExporter/fileWriter.ts (1)
47-67: Standardize file extension to .yaml.Comments and examples mix
.ymland.yaml. The code uses.yaml(line 64), but examples show.yml. Standardize to.yamlthroughout.Update examples:
# Optional: Override config file path # Supports ${"$"}{BUNDLER} substitution (webpack/rspack) - # config: ${"$"}{BUNDLER}.config.ts # Defaults to auto-detected file + # config: webpack.config.ts # Defaults to auto-detected fileApply this diff:
* webpack-development-client.yaml * rspack-production-server.yaml - * webpack-dev-client.yaml (with build name) - * rspack-cypress-dev-server.yaml (with build name) + * webpack-dev-client.yaml (with build name) + * rspack-cypress-dev-server.yaml (with build name)package/configExporter/configFile.ts (2)
85-134: Strengthen validation to reject null, arrays, and invalid value types.Current validation is too permissive:
buildscould be null or an arraybundler_envvalues aren't validated as string|booleanenvironmentvalues aren't validated as stringsoutputselements aren't validated as stringsconfigisn't validated as a stringApply this diff:
private validate(config: BundlerConfigFile): void { - if (!config.builds || typeof config.builds !== "object") { + if (!config.builds || typeof config.builds !== "object" || Array.isArray(config.builds)) { throw new Error("Config file must contain a 'builds' object") } // Validate each build for (const [name, build] of Object.entries(config.builds)) { + if (!build || typeof build !== "object" || Array.isArray(build)) { + throw new Error(`Build '${name}' must be an object`) + } + if ( build.bundler && - if (build.bundler_env && typeof build.bundler_env !== "object") { + if (build.bundler_env) { + if (typeof build.bundler_env !== "object" || build.bundler_env === null || Array.isArray(build.bundler_env)) { + throw new Error(`Invalid bundler_env in build '${name}'. Must be an object.`) + } + for (const [k, v] of Object.entries(build.bundler_env)) { + if (typeof v !== "string" && typeof v !== "boolean") { + throw new Error(`Invalid bundler_env['${k}'] in build '${name}'. Must be string or boolean.`) + } + } - throw new Error( - `Invalid bundler_env in build '${name}'. Must be an object.` - ) } - if (build.environment && typeof build.environment !== "object") { + if (build.environment) { + if (typeof build.environment !== "object" || build.environment === null || Array.isArray(build.environment)) { + throw new Error(`Invalid environment in build '${name}'. Must be an object.`) + } + for (const [k, v] of Object.entries(build.environment)) { + if (typeof v !== "string") { + throw new Error(`Invalid environment['${k}'] in build '${name}'. Must be a string.`) + } + } - throw new Error( - `Invalid environment in build '${name}'. Must be an object.` - ) } - if (build.outputs && !Array.isArray(build.outputs)) { + if (build.outputs) { + if (!Array.isArray(build.outputs) || !build.outputs.every(o => typeof o === "string")) { + throw new Error(`Invalid outputs in build '${name}'. Must be an array of strings.`) + } - throw new Error( - `Invalid outputs in build '${name}'. Must be an array of strings.` - ) } + + if (build.config && typeof build.config !== "string") { + throw new Error(`Invalid config in build '${name}'. Must be a string path.`) + } } }
207-218: Coerce values to strings before expansion.YAML scalars may be non-strings (numbers, booleans, null). Calling
.replace()on these will throw.Apply this diff:
for (const [key, value] of Object.entries(vars)) { - expanded[key] = this.expandString(value, bundler) + expanded[key] = this.expandString(String(value), bundler) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bundler-config-enhancement-spec.md(1 hunks)package/configExporter/cli.ts(13 hunks)package/configExporter/configFile.ts(1 hunks)package/configExporter/fileWriter.ts(1 hunks)package/configExporter/index.ts(1 hunks)package/configExporter/types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package/configExporter/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
End every file with a trailing newline character
Files:
package/configExporter/types.tspackage/configExporter/fileWriter.tspackage/configExporter/configFile.tspackage/configExporter/cli.tsbundler-config-enhancement-spec.md
🧬 Code graph analysis (3)
package/configExporter/types.ts (1)
package/configExporter/index.ts (3)
BundlerConfigFile(6-6)BuildConfig(7-7)ResolvedBuildConfig(8-8)
package/configExporter/configFile.ts (1)
package/configExporter/types.ts (3)
BundlerConfigFile(45-48)ExportOptions(1-20)ResolvedBuildConfig(59-67)
package/configExporter/cli.ts (2)
package/configExporter/configFile.ts (2)
ConfigFileLoader(17-284)generateSampleConfigFile(290-451)package/configExporter/types.ts (1)
ExportOptions(1-20)
🪛 markdownlint-cli2 (0.18.1)
bundler-config-enhancement-spec.md
216-216: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (7)
package/configExporter/types.ts (1)
15-67: LGTM!The type definitions are well-structured and provide clear contracts for the config file schema and resolution logic. The optional fields and union types appropriately model the different configuration scenarios.
package/configExporter/cli.ts (2)
200-228: LGTM!The init command implementation correctly generates the sample config file and provides helpful next-steps guidance.
230-234: LGTM!The list-builds command delegates to the loader appropriately.
package/configExporter/configFile.ts (4)
60-83: LGTM!Using
FAILSAFE_SCHEMAprevents code execution via YAML parsing, which is the correct security posture for untrusted config files.Based on learnings
35-45: LGTM!Path traversal protection correctly prevents configs from outside the project directory.
144-196: LGTM!The build resolution logic correctly applies precedence for bundler selection and properly expands environment variables.
290-451: LGTM!The sample config file is comprehensive and well-documented with clear examples for various build scenarios.
bundler-config-enhancement-spec.md
Outdated
| ``` | ||
| Available builds in .bundler-config.yml: | ||
| dev | ||
| Description: Build admin and consumer bundles for dev env | ||
| Bundler: webpack (default) | ||
| Outputs: client, server | ||
| dev-server | ||
| Description: Build server bundle for dev env | ||
| Bundler: webpack (default) | ||
| Outputs: server | ||
| cypress-dev | ||
| Description: Build for Cypress development | ||
| Bundler: webpack (default) | ||
| Outputs: client, server | ||
| prod-server | ||
| Description: Build server bundle for production | ||
| Bundler: webpack (default) | ||
| Outputs: server | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
The code block is missing a language identifier, which reduces readability and tooling support.
Apply this diff:
-```
+```text
Available builds in .bundler-config.yml:🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
216-216: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In bundler-config-enhancement-spec.md around lines 216 to 238, the fenced code
block lacks a language identifier; update the opening fence to include "text"
(i.e., change ``` to ```text) so the block becomes ```text and leave the closing
fence as-is to improve readability and tooling support.
Code Review: YAML Configuration SupportThank you for this comprehensive PR! Below is my detailed review. Strengths
Critical Issues1. Missing Type DefinitionsThe types.ts file is missing several type definitions referenced in the code (BundlerConfigFile, BuildConfig, ResolvedBuildConfig, buildName field). Action: Verify all types are properly defined in types.ts 2. FileWriter Signature MismatchgenerateFilename is called with 5 parameters (including buildName) but only accepts 4. Action: Update signature to include optional buildName parameter 3. Incomplete Validation LogicValidation for --build flag only checks config file existence when options.configFile is undefined. Fix: Always validate when --build is specified Significant Issues4. Environment Variable OverwritingEnvironment variables from build config overwrite existing ones without warning. Consider: Add precedence check or logging 5. Type Assertion Without Validationcli.ts:485-486 uses unsafe type assertion for output types. Fix: Validate before casting 6. Index Out of BoundsAccessing buildOutputs[index] without bounds checking. Fix: Add bounds check Code Quality
Testing Concerns
Additional
SummaryMust Fix: Types, FileWriter signature, validation logic, trailing newlines, linting, tests Should Fix: Error handling, type validation, bounds checking, env var warnings Recommendation: Request Changes - Address critical issues and add test coverage before approval. Great work on the design and backward compatibility! Once issues are addressed, this will be valuable. |
Critical Fixes: - Fix --build validation to always check config file existence - Add bounds checking and type validation for buildOutputs array - Add try-catch error handling to runListBuildsCommand - Improve error messages with specific config file paths Type Safety: - Add explicit validation before casting output types - Check array bounds before accessing buildOutputs[index] - Validate output values are valid config types (client/server/all) Error Handling: - Return proper exit codes from runListBuildsCommand - Better error messages for missing config files - Warn on invalid output types instead of silent failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
package/configExporter/cli.ts (2)
135-147: Restore support for--annotate.Legacy callers rely on
--annotate, but we only parse--no-annotate, so the flag silently stops working. Please add the explicit handler to keep backward compatibility.Apply this diff:
} else if (arg === "--no-annotate") { options.annotate = false + } else if (arg === "--annotate") { + options.annotate = true
381-408: Config build NODE_ENV never applied.
options.envdefaults to"development", so!options.envis never true and the build’s NODE_ENV can’t win. This forces every--buildexport to stay in dev even when the config asks for prod/test. Track whether the user passed--envand only block the override when they did.Apply this diff:
-function parseArguments(args: string[]): ExportOptions { - const options: ExportOptions = { +function parseArguments(args: string[]): ExportOptions { + const options: ExportOptions & { envWasExplicit?: boolean } = { bundler: undefined, - env: "development", + env: "development", @@ } else if (arg.startsWith("--env=")) { const env = parseValue(arg, "--env=") if (env !== "development" && env !== "production" && env !== "test") { throw new Error( `Invalid environment '${env}'. Must be 'development', 'production', or 'test'.` ) } options.env = env + options.envWasExplicit = true @@ - return options + return options } @@ - if (resolvedBuild.environment.NODE_ENV && !options.env) { + const envWasExplicit = + (options as ExportOptions & { envWasExplicit?: boolean }).envWasExplicit === true + if (resolvedBuild.environment.NODE_ENV && !envWasExplicit) { env = resolvedBuild.environment.NODE_ENV as | "development" | "production" | "test" }
🧹 Nitpick comments (1)
package/index.d.ts (1)
20-22: Keep env types narrow for better safety.
railsEnvandnodeEnvare now typed asany, which drops the string semantics we rely on across the codebase. Please keep these asstring | undefined(or the narrowest accurate type) so downstream consumers retain proper type checking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/configExporter/cli.ts(13 hunks)package/index.d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
End every file with a trailing newline character
Files:
package/index.d.tspackage/configExporter/cli.ts
🧬 Code graph analysis (1)
package/configExporter/cli.ts (2)
package/configExporter/configFile.ts (2)
ConfigFileLoader(17-284)generateSampleConfigFile(290-451)package/configExporter/types.ts (1)
ExportOptions(1-20)
The spec document is already available in issue #687. Removing it from the PR to avoid prettier formatting conflicts in CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
The main branch has been updated with significant changes to CLI parsing (migration to yargs). To avoid complex merge conflicts, I'll create a fresh PR based on the latest main with all the YAML configuration features integrated properly with the new yargs approach. This PR will be closed in favor of the new one. |
1 participant
Summary
Implements #687 - adds YAML configuration file support to
bin/export-bundler-config, allowing users to define and version control their build configurations.Key Features
--initcommand to generate sample.bundler-config.yml--build=<name>flag to export specific build from config file--list-buildscommand to show available builds--webpackand--rspackflags for easy bundler switching${VAR},${VAR:-default},${BUNDLER})bundler_envto CLI args conversionbundler-build-type.ext)YAML Config Example
Usage Examples
Implementation Details
package/configExporter/configFile.ts- Config file loading and parsingpackage/configExporter/types.ts- Schema types for config filepackage/configExporter/cli.ts- CLI argument parsing and integrationpackage/configExporter/fileWriter.ts- Build-name file naming supportbundler-config-enhancement-spec.md- Full specification documentTest Plan
Breaking Changes
None - fully backward compatible
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation