feat: add zero-config auto mode to GitHub Action#2797
Conversation
🦋 Changeset detectedLatest commit: c21e335 The changes in this PR will be included in the next version bump. This PR includes changesets to release 31 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a zero‑config "auto" mode to the GitHub Action: file discovery, file‑type detection, compressor selection, output‑dir validation, chunked processing, and grouped reporting; expands action inputs, docs, wildcards API, CI test jobs, and extensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant A as Action
participant D as Discover
participant T as Detector
participant S as Selector
participant M as Minifier
participant R as Reporter
U->>A: Trigger action (auto:true, patterns?, output-dir?, dry-run?)
A->>D: discoverFiles(patterns, ignore, cwd, dryRun)
D-->>A: file list
A->>T: groupFilesByType(file list)
T-->>A: grouped files by FileType
loop per FileType
A->>S: selectCompressor(FileType)
S-->>A: CompressorSelection
A->>M: process files (chunkArray -> runMinification)
M-->>A: FileResult entries
end
A->>R: generateAutoModeSummary(all results, inputs)
R-->>A: rendered summary
A-->>U: completion status / outputs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Latest suggestions up to a8273bb
Previous suggestions✅ Suggestions up to commit a7a1c1f
|
|||||||||||||||||||||||||||||||||
Greptile OverviewGreptile SummaryThis PR adds zero-config auto mode to the GitHub Action, enabling automatic file discovery and minification without explicit input/output configuration. The implementation includes comprehensive security hardening for path handling and extensive test coverage. Key Changes:
Test Coverage:
Security Measures:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant run
participant parseInputs
participant runAutoMode
participant discoverFiles
participant validateOutputDir
participant groupFilesByType
participant resolveCompressor
participant minify
participant setMinifyOutputs
User->>run: Trigger GitHub Action
run->>parseInputs: Parse and validate inputs
parseInputs->>parseInputs: Check if auto mode enabled
alt Auto Mode Disabled
parseInputs-->>parseInputs: Validate input/output required
end
parseInputs->>validateOutputDir: Validate output-dir path
validateOutputDir->>validateOutputDir: Check for ".." segments
validateOutputDir->>validateOutputDir: Check if absolute path
validateOutputDir->>validateOutputDir: Check overlap with source patterns
validateOutputDir-->>parseInputs: Validation result
parseInputs-->>run: Return ActionInputs
alt inputs.auto === true
run->>runAutoMode: Execute auto mode
runAutoMode->>discoverFiles: Discover files with patterns
discoverFiles->>discoverFiles: Apply default patterns/ignores
discoverFiles-->>runAutoMode: Return file list
alt No files found
runAutoMode->>setMinifyOutputs: Set empty outputs
runAutoMode-->>run: Return
end
alt Dry run mode
runAutoMode->>runAutoMode: Log files and exit
runAutoMode-->>run: Return
end
runAutoMode->>groupFilesByType: Group files by extension
groupFilesByType-->>runAutoMode: Return grouped files
loop For each file type
runAutoMode->>resolveCompressor: Check compressor availability
resolveCompressor-->>runAutoMode: Compressor found or error
end
runAutoMode->>runAutoMode: Create output directory
loop Process files in chunks of 4
par Parallel processing within chunk
runAutoMode->>minify: Minify file
minify-->>runAutoMode: Return minified content
end
end
runAutoMode->>setMinifyOutputs: Set results and totals
runAutoMode-->>run: Complete
else inputs.auto === false
run->>run: Execute explicit mode (original flow)
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
packages/action/src/index.ts
Outdated
| const fileType = grouped.js.includes(file) | ||
| ? "js" | ||
| : grouped.css.includes(file) | ||
| ? "css" | ||
| : grouped.html.includes(file) | ||
| ? "html" | ||
| : grouped.json.includes(file) | ||
| ? "json" | ||
| : grouped.svg.includes(file) | ||
| ? "svg" | ||
| : "unknown"; |
There was a problem hiding this comment.
use detectFileType(file) instead of this complex nested ternary chain
| const fileType = grouped.js.includes(file) | |
| ? "js" | |
| : grouped.css.includes(file) | |
| ? "css" | |
| : grouped.html.includes(file) | |
| ? "html" | |
| : grouped.json.includes(file) | |
| ? "json" | |
| : grouped.svg.includes(file) | |
| ? "svg" | |
| : "unknown"; | |
| const fileType = detectFileType(file); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/action/src/index.ts
Line: 168:178
Comment:
use `detectFileType(file)` instead of this complex nested ternary chain
```suggestion
const fileType = detectFileType(file);
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
srod
force-pushed
the
feat/zero-config-action
branch
from
baca889 to
3554ebf
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 30 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="docs/src/content/docs/github-action.md">
<violation number="1" location="docs/src/content/docs/github-action.md:44">
P3: The dotfiles ignore glob includes a trailing space, so it won’t match hidden files as intended. Remove the whitespace so the pattern is valid.</violation>
</file>
<file name="packages/action/src/inputs.ts">
<violation number="1" location="packages/action/src/inputs.ts:72">
P2: Auto mode skips outputDir validation when patterns aren’t provided, even though default patterns are still used. This allows output directories under `src/`/`app/` etc. to be rediscovered and re-minified. Validate against the effective patterns in auto mode, not only custom patterns.</violation>
</file>
<file name="packages/action/src/validate.ts">
<violation number="1" location="packages/action/src/validate.ts:32">
P2: The validation compares raw strings without resolving relative segments. An output-dir like "../src" (or on Windows, a path with backslashes) bypasses this check, so the output can still overlap with the source tree and trigger the infinite-loop risk this function is meant to prevent.</violation>
</file>
<file name="packages/action/src/index.ts">
<violation number="1" location="packages/action/src/index.ts:81">
P2: Auto mode never checks `fail-on-increase`/`min-reduction` thresholds, so the action can report success even when total reduction violates configured thresholds.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
🤖 Fix all issues with AI agents
In `@packages/action/src/discover.ts`:
- Around line 69-81: The generateOutputPath function currently uses
path.join(outputDir, inputFile) which will ignore outputDir when inputFile is
absolute; update generateOutputPath to detect absolute input paths (using
path.isAbsolute or equivalent) and convert them to a path relative to the
repository or to outputDir (e.g., via path.relative or stripping leading path
separators) before joining so the resultant path always resides under outputDir;
ensure you still preserve the input file's directory structure and handle edge
cases like inputFile === path.sep or empty relative results in the function
generateOutputPath.
In `@packages/action/src/index.ts`:
- Around line 112-128: runAutoMode currently logs a warning and returns when
discoverFiles finds no files, which leaves expected action outputs unset; update
runAutoMode to explicitly set the action outputs to sensible defaults (e.g.,
file count = 0, processed files = empty list, summary/message = "No files
found") before returning and also generate the same summary/notice that would
appear when files are processed so downstream consumers receive consistent
outputs; locate and update the runAutoMode function and the places that normally
call core.setOutput / createSummary (or the action's output helpers) to emit
these default values when files.length === 0.
In `@packages/action/src/validate.ts`:
- Around line 7-14: The JSDoc for the exported function that validates the
output directory (the function taking parameters outputDir and patterns) is
missing a `@returns` tag; update its comment to include an `@returns` describing the
function's return type and behavior (e.g., "void" or "void — throws Error if
outputDir is inside any source pattern"), so the exported JSDoc explicitly
documents the return value and exception behavior.
♻️ Duplicate comments (5)
docs/src/content/docs/github-action.md (1)
40-45: Remove trailing space in dotfiles ignore glob.The code span contains a trailing space, which breaks the glob and triggers MD038.
🧹 Proposed fix
- - `**/.* ` (hidden files) + - `**/.*` (hidden files)packages/action/src/validate.ts (1)
23-33: Normalize paths before overlap checks.Raw string comparison can be bypassed with
..segments or Windows separators, reintroducing the infinite-loop risk.🔒 Proposed fix
+import path from "node:path"; + export function validateOutputDir(outputDir: string, patterns: string[]): void { for (const pattern of patterns) { // Extract base directory from glob pattern (everything before first *) const baseDir = pattern.split("*")[0]; @@ if (baseDir) { - const baseDirNormalized = baseDir.endsWith("/") - ? baseDir.slice(0, -1) - : baseDir; - const outputDirNormalized = outputDir.endsWith("/") - ? outputDir.slice(0, -1) - : outputDir; - - if ( - outputDirNormalized === baseDirNormalized || - outputDirNormalized.startsWith(`${baseDirNormalized}/`) - ) { + const baseDirNormalized = baseDir.endsWith("/") + ? baseDir.slice(0, -1) + : baseDir; + const outputDirNormalized = outputDir.endsWith("/") + ? outputDir.slice(0, -1) + : outputDir; + const baseDirResolved = path.resolve(baseDirNormalized); + const outputDirResolved = path.resolve(outputDirNormalized); + const relative = path.relative(baseDirResolved, outputDirResolved); + + if ( + relative === "" || + (!relative.startsWith("..") && !path.isAbsolute(relative)) + ) { throw new Error( `output-dir cannot be inside source pattern: '${outputDir}' overlaps with '${pattern}'` ); } } } }packages/action/src/inputs.ts (1)
72-74: Validate outputDir against effective patterns even when none are provided.
Auto mode still uses default discovery patterns, but validation is skipped whenpatternsis empty — so an outputDir undersrc//app/can be rediscovered.🔧 Proposed fix
-import { validateOutputDir } from "./validate.ts"; +import { DEFAULT_PATTERNS } from "./discover.ts"; +import { validateOutputDir } from "./validate.ts"; @@ - if (auto && patterns.length > 0) { - validateOutputDir(outputDir, patterns); - } + if (auto) { + const effectivePatterns = + patterns.length > 0 ? patterns : DEFAULT_PATTERNS; + validateOutputDir(outputDir, effectivePatterns); + }packages/action/src/index.ts (2)
168-178: UsedetectFileType(file)instead of nested ternary chain.This complex nested ternary is hard to read and duplicates logic already available in
detectFileType. The function is already imported from./autoDetect.ts.Proposed fix
- const fileType = grouped.js.includes(file) - ? "js" - : grouped.css.includes(file) - ? "css" - : grouped.html.includes(file) - ? "html" - : grouped.json.includes(file) - ? "json" - : grouped.svg.includes(file) - ? "svg" - : "unknown"; + const fileType = detectFileType(file);
275-283: Auto mode skips threshold validation.
runExplicitModecallscheckThresholds(line 96) and fails the action if thresholds are violated.runAutoModeskips this check entirely, meaningfail-on-increaseandmin-reductioninputs are silently ignored in auto mode.Proposed fix
setMinifyOutputs(minifyResult); if (inputs.reportSummary) { await generateSummary(minifyResult); } + const thresholdError = checkThresholds(totalReduction, inputs); + if (thresholdError) { + setFailed(thresholdError); + return; + } + info( `✅ Auto mode complete! Processed ${allResults.length} files with ${totalReduction.toFixed(1)}% total reduction in ${totalTimeMs}ms` );
🧹 Nitpick comments (8)
packages/utils/__tests__/wildcards-ignore.test.ts (2)
9-10: Import paths should use.tsextension per coding guidelines.The local imports use
.jsextension, but the coding guidelines specify using.tsextension for local import paths.Proposed fix
-import type { WildcardOptions } from "../src/wildcards.js"; -import { DEFAULT_IGNORES, wildcards } from "../src/wildcards.js"; +import type { WildcardOptions } from "../src/wildcards.ts"; +import { DEFAULT_IGNORES, wildcards } from "../src/wildcards.ts";
101-113: Test name doesn't match the test behavior.The test is named "should use DEFAULT_IGNORES when no ignore option provided" but actually tests explicit usage of
DEFAULT_IGNORES. The comment on line 104 correctly states the intent ("Test that we can use DEFAULT_IGNORES explicitly"), but the test name is misleading.Consider renaming to clarify the actual behavior being tested:
Proposed fix
- test("should use DEFAULT_IGNORES when no ignore option provided", () => { + test("should allow explicit use of DEFAULT_IGNORES as ignore option", () => {packages/utils/src/wildcards.ts (1)
45-64: Missing@returnsJSDoc tag on exported function.Per coding guidelines, all exported functions must have JSDoc comments with
@paramand@returnstags. Thewildcardsfunction is missing the@returnstag.Proposed fix
/** * Handle wildcards in a path, get the real path of each file. * `@param` input - Path with wildcards * `@param` options - Options object or string publicFolder for backward compatibility + * `@returns` Object containing the resolved input file paths */ export function wildcards(packages/action/src/benchmark.ts (1)
48-53: Minor inconsistency: use localinputvariable in error message.Line 50 uses
inputs.inputwhile the rest of the function uses the destructuredinputvariable. For consistency, use the local variable.Suggested fix
if (!fileResult) { throw new Error( - `Benchmark failed: no results for input "${inputs.input}". ` + + `Benchmark failed: no results for input "${input}". ` + "Check that the file exists and at least one compressor succeeded." ); }.github/workflows/test-action.yml (1)
104-108: Actionlint false positives - new inputs are defined in action.yml.The static analysis errors for undefined inputs (
auto,patterns,output-dir,ignore,dry-run) are false positives. These inputs are correctly defined inaction.yml(lines 15-32) in this PR. The linter likely ran against a stale version.However, these explicit parameters are redundant for explicit-mode tests since
auto: 'false'anddry-run: 'false'are already the defaults. Consider removing them to reduce noise.Optional: Remove redundant defaults
- name: Minify with terser id: minify-terser uses: ./ with: input: "test-action/input.js" output: "test-action/output.min.js" compressor: "terser" - auto: 'false' - patterns: '' - output-dir: 'dist' - ignore: '' - dry-run: 'false'packages/action/src/index.ts (3)
153-153: Magic number for concurrency limit.The chunk size
4is hardcoded without explanation. Consider extracting this to a named constant or making it configurable for clarity and future flexibility.Suggested improvement
+const AUTO_MODE_CONCURRENCY = 4; + // Process files with concurrency limit const allResults: FileResult[] = []; const failures: { file: string; error: string }[] = []; -const chunks = chunkArray(files, 4); +const chunks = chunkArray(files, AUTO_MODE_CONCURRENCY);
180-184: Redundant compressor resolution per file.
resolveCompressoris called twice for the same compressor: once in the validation loop (lines 133-143) and again here for each file. Since compressor resolution may involve async I/O, consider caching resolved compressors by type to avoid redundant lookups.Suggested optimization
+ // Pre-resolve compressors for each file type + const resolvedCompressors = new Map<FileType, { compressorFn: unknown; label: string }>(); for (const [type, typeFiles] of Object.entries(grouped)) { if (typeFiles.length === 0) continue; const { compressor, package: pkg } = selectCompressor(type as FileType); try { - await resolveCompressor(compressor); + const resolved = await resolveCompressor(compressor); + resolvedCompressors.set(type as FileType, { + compressorFn: resolved.compressor, + label: resolved.label, + }); } catch { throw new Error( `Compressor for ${type} files not found. Run: npm install ${pkg}` ); } }Then use the cached value in the file processing loop instead of calling
resolveCompressoragain.
217-217: Unnecessary type assertion.The
as FileResult & { compressor: string }assertion suggests a type mismatch. IfFileResultalready includescompressor, the assertion is redundant. If not, consider extendingFileResultin the type definition rather than using inline intersection.
| /** | ||
| * Validates that the output directory is not inside any source pattern. | ||
| * Prevents infinite loop risk when output overlaps with source files. | ||
| * | ||
| * @param outputDir - The output directory path | ||
| * @param patterns - Array of source file patterns (glob patterns) | ||
| * @throws Error if outputDir is inside any source pattern | ||
| */ |
There was a problem hiding this comment.
Add @returns to exported JSDoc.
Exported functions must include @returns in JSDoc.
📝 Proposed fix
/**
* Validates that the output directory is not inside any source pattern.
* Prevents infinite loop risk when output overlaps with source files.
*
* `@param` outputDir - The output directory path
* `@param` patterns - Array of source file patterns (glob patterns)
+ * `@returns` void
* `@throws` Error if outputDir is inside any source pattern
*/📝 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.
| /** | |
| * Validates that the output directory is not inside any source pattern. | |
| * Prevents infinite loop risk when output overlaps with source files. | |
| * | |
| * @param outputDir - The output directory path | |
| * @param patterns - Array of source file patterns (glob patterns) | |
| * @throws Error if outputDir is inside any source pattern | |
| */ | |
| /** | |
| * Validates that the output directory is not inside any source pattern. | |
| * Prevents infinite loop risk when output overlaps with source files. | |
| * | |
| * `@param` outputDir - The output directory path | |
| * `@param` patterns - Array of source file patterns (glob patterns) | |
| * `@returns` void | |
| * `@throws` Error if outputDir is inside any source pattern | |
| */ |
🤖 Prompt for AI Agents
In `@packages/action/src/validate.ts` around lines 7 - 14, The JSDoc for the
exported function that validates the output directory (the function taking
parameters outputDir and patterns) is missing a `@returns` tag; update its comment
to include an `@returns` describing the function's return type and behavior (e.g.,
"void" or "void — throws Error if outputDir is inside any source pattern"), so
the exported JSDoc explicitly documents the return value and exception behavior.
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/action/src/inputs.ts">
<violation number="1" location="packages/action/src/inputs.ts:64">
P3: The path traversal guard is too broad: `includes("..")` blocks legitimate directory names containing `..` even when they are not traversal segments. This can reject valid `output-dir` values. Check for `..` path segments instead of substring matches.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/action/src/index.ts`:
- Around line 38-43: The chunkArray function lacks validation for the size
parameter and will loop infinitely when size is zero or behave incorrectly for
negative values; add a defensive check at the start of chunkArray(size) to
validate that size is a positive integer (e.g., if (!Number.isInteger(size) ||
size <= 0) throw new TypeError or return a clear error), so the function fails
fast with an informative message; keep the rest of the logic (the for loop and
slicing) unchanged and reference the chunkArray<T>(array: T[], size: number)
signature when making the change.
♻️ Duplicate comments (2)
packages/action/src/index.ts (2)
121-124: Set outputs when no files are discovered.
Currently auto mode returns without outputs; downstream steps won’t see expected values.🛠️ Suggested default outputs
if (files.length === 0) { warning("No files found matching patterns"); + setMinifyOutputs({ + files: [], + compressor: "auto", + totalOriginalSize: 0, + totalMinifiedSize: 0, + totalReduction: 0, + totalTimeMs: 0, + }); return; }
267-284: Auto mode should enforce reduction thresholds.
Explicit mode callscheckThresholds, but auto mode doesn’t—sofail-on-increase/min-reductioncan be bypassed.🛠️ Suggested threshold check
if (inputs.reportSummary) { await generateSummary(minifyResult); } + const thresholdError = checkThresholds(minifyResult.totalReduction, inputs); + if (thresholdError) { + setFailed(thresholdError); + return; + } + info( `✅ Auto mode complete! Processed ${allResults.length} files with ${totalReduction.toFixed(1)}% total reduction in ${totalTimeMs}ms` );
srod
force-pushed
the
feat/zero-config-action
branch
from
09e7477 to
f74b577
Compare
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/action/src/inputs.ts">
<violation number="1" location="packages/action/src/inputs.ts:64">
P2: The new validation normalizes the path before checking for "..", which strips mid-path traversal segments (e.g., "dist/../build"), letting disallowed ".." segments slip through. Validate the raw segments instead of the normalized path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/action/src/index.ts`:
- Around line 63-67: The JSDoc for exported functions is missing `@returns` tags;
update the JSDoc for runExplicitMode, runAutoMode, and run to include an
`@returns` description that matches each function's actual return type (e.g.,
Promise<void> or the specific value returned) and a brief description of what
the caller receives; ensure the new `@returns` tag follows the existing `@param`
tags and mirrors the function signatures in index.ts so doc comments remain
accurate for exported APIs.
🧹 Nitpick comments (2)
packages/action/src/index.ts (2)
150-160: Preserve underlying error context when a compressor fails to resolve.
The barecatchdrops the original error details. Capture it and useinstanceof Errorto append useful context. As per coding guidelines, error handling should useerr instanceof Error.🔧 Suggested adjustment
- } catch { - throw new Error( - `Compressor for ${type} files not found. Run: npm install ${pkg}` - ); - } + } catch (err) { + const detail = + err instanceof Error && err.message + ? ` (${err.message})` + : ""; + throw new Error( + `Compressor for ${type} files not found. Run: npm install ${pkg}${detail}` + ); + }
258-273: Auto-mode totalTimeMs likely overstates real runtime.
Summing per-file timeMs inflates the total when files are processed in parallel. Consider measuring wall-clock time around the whole auto-mode processing and reporting that instead (or alongside).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/action/src/discover.ts`:
- Around line 80-91: Normalize the input path (use path.normalize on inputFile /
cleanInput) before stripping traversal sequences so internal ".." segments are
resolved, then remove any leading "../" or "..\" sequences and use
path.join(outputDir, cleanInput); finally verify the final path is inside
outputDir (e.g., with path.relative(outputDir, finalPath) and reject or throw if
it starts with ".." or is absolute) to block any nested traversal escaping;
reference variables: cleanInput, inputFile, outputDir, and path.join.
🧹 Nitpick comments (1)
packages/action/__tests__/discover.test.ts (1)
185-215: Add a regression test for nested traversal.
If you hardengenerateOutputPath, add a case likefoo/../../bar.jsto ensure it can’t escape the output directory.✅ Suggested test
test("should sanitize directory traversal in input path", () => { const result = generateOutputPath("../../test.log", "dist"); expect(result).toBe(path.join("dist", "test.log")); }); + + test("should sanitize nested traversal in input path", () => { + const result = generateOutputPath("foo/../../bar.js", "dist"); + expect(result).toBe(path.join("dist", "bar.js")); + });
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/action/src/validate.ts`:
- Around line 20-35: The loop over patterns in validate.ts incorrectly skips
patterns whose baseDir is empty (e.g., patterns starting with "**/") because
baseDir is falsy; update the logic in the for (const pattern of patterns) loop
so that baseDir is normalized to a valid directory (for example use
path.resolve(baseDir || ".") or treat empty baseDir as the repository/workspace
root) before computing absBaseDir and rel, ensuring the overlap check that
computes isInside (using absBaseDir, absOutputDir and
path.relative/path.isAbsolute) runs for every pattern rather than being gated on
baseDir truthiness.
♻️ Duplicate comments (1)
packages/action/src/validate.ts (1)
9-16: Add missing@returnsin exported JSDoc.
This exported function is missing a@returnstag, which is required forpackages/*/src/**/*.ts. As per coding guidelines, please add it.📝 Suggested update
/** * Validates that the output directory is not inside any source pattern. * Prevents infinite loop risk when output overlaps with source files. * * `@param` outputDir - The output directory path * `@param` patterns - Array of source file patterns (glob patterns) + * `@returns` void * `@throws` Error if outputDir is inside any source pattern */
🧹 Nitpick comments (1)
packages/action/src/index.ts (1)
150-205: Cache per‑type compressor resolution to avoid repeated work.
You already group by type, but still calldetectFileType/selectCompressor/resolveCompressorfor each file. Caching resolved compressors once per type would reduce repeated module resolution and per-file overhead.♻️ Possible refactor sketch
+ const compressorCache = new Map< + FileType, + { compressor: unknown; label: string; type?: string } + >(); // Check compressor availability first for (const [type, typeFiles] of Object.entries(grouped)) { if (typeFiles.length === 0) continue; - const { compressor, package: pkg } = selectCompressor(type as FileType); + const { compressor, package: pkg, type: minifyType } = selectCompressor( + type as FileType + ); try { - await resolveCompressor(compressor); + const resolved = await resolveCompressor(compressor); + compressorCache.set(type as FileType, { + ...resolved, + type: minifyType, + }); } catch { throw new Error( `Compressor for ${type} files not found. Run: npm install ${pkg}` ); } } chunk.map(async (file) => { const outputPath = generateOutputPath(file, inputs.outputDir); const inputPath = path.join(inputs.workingDirectory, file); const fullOutputPath = path.join( inputs.workingDirectory, outputPath ); // Ensure output directory exists await mkdir(path.dirname(fullOutputPath), { recursive: true }); - const fileType = detectFileType(file); - - const { compressor, type } = selectCompressor( - fileType as FileType - ); - const { compressor: compressorFn, label } = - await resolveCompressor(compressor); + const fileType = detectFileType(file) as FileType; + const cached = compressorCache.get(fileType); + if (!cached) { + throw new Error(`Unsupported file type: ${file}`); + } + const { compressor: compressorFn, label, type } = cached;
| for (const pattern of patterns) { | ||
| // Extract base directory from glob pattern (everything before first *) | ||
| const baseDir = pattern.split("*")[0]; | ||
|
|
||
| if (baseDir) { | ||
| const absBaseDir = path.resolve(baseDir); | ||
|
|
||
| // Check if output dir is same as or inside base dir | ||
| const rel = path.relative(absBaseDir, absOutputDir); | ||
|
|
||
| // If rel is empty, they are same. | ||
| // If rel does not start with '..', output is inside base. | ||
| // Exception: check if rel is absolute (different drives on Windows) | ||
| const isInside = | ||
| rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel)); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the validate.ts file
find . -path "*/packages/action/src/validate.ts" -type fRepository: srod/node-minify
Length of output: 93
🏁 Script executed:
# Read the file to see the actual code
cat -n packages/action/src/validate.tsRepository: srod/node-minify
Length of output: 1840
🏁 Script executed:
# Let's also check the full context of the validation function to understand its purpose
head -100 packages/action/src/validate.ts | cat -nRepository: srod/node-minify
Length of output: 1840
Leading **/ globs bypass overlap checks, skipping validation entirely.
When a pattern starts with **/ (e.g., **/*.js), baseDir becomes an empty string, which is falsy, so the validation block never executes. This allows output directories that still match the pattern to pass unchecked, defeating the infinite loop prevention. Patterns with no static prefix must also be handled to ensure the overlap check always runs.
🛠️ Suggested fix
- const baseDir = pattern.split("*")[0];
+ const baseDir = pattern.split("*")[0];
+ const hasDeepGlob =
+ pattern.startsWith("**/") || pattern.startsWith("**\\");
+ const effectiveBaseDir = baseDir || (hasDeepGlob ? "." : "");
- if (baseDir) {
- const absBaseDir = path.resolve(baseDir);
+ if (effectiveBaseDir) {
+ const absBaseDir = path.resolve(effectiveBaseDir);📝 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.
| for (const pattern of patterns) { | |
| // Extract base directory from glob pattern (everything before first *) | |
| const baseDir = pattern.split("*")[0]; | |
| if (baseDir) { | |
| const absBaseDir = path.resolve(baseDir); | |
| // Check if output dir is same as or inside base dir | |
| const rel = path.relative(absBaseDir, absOutputDir); | |
| // If rel is empty, they are same. | |
| // If rel does not start with '..', output is inside base. | |
| // Exception: check if rel is absolute (different drives on Windows) | |
| const isInside = | |
| rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel)); | |
| for (const pattern of patterns) { | |
| // Extract base directory from glob pattern (everything before first *) | |
| const baseDir = pattern.split("*")[0]; | |
| const hasDeepGlob = | |
| pattern.startsWith("**/") || pattern.startsWith("**\\"); | |
| const effectiveBaseDir = baseDir || (hasDeepGlob ? "." : ""); | |
| if (effectiveBaseDir) { | |
| const absBaseDir = path.resolve(effectiveBaseDir); | |
| // Check if output dir is same as or inside base dir | |
| const rel = path.relative(absBaseDir, absOutputDir); | |
| // If rel is empty, they are same. | |
| // If rel does not start with '..', output is inside base. | |
| // Exception: check if rel is absolute (different drives on Windows) | |
| const isInside = | |
| rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel)); |
🤖 Prompt for AI Agents
In `@packages/action/src/validate.ts` around lines 20 - 35, The loop over patterns
in validate.ts incorrectly skips patterns whose baseDir is empty (e.g., patterns
starting with "**/") because baseDir is falsy; update the logic in the for
(const pattern of patterns) loop so that baseDir is normalized to a valid
directory (for example use path.resolve(baseDir || ".") or treat empty baseDir
as the repository/workspace root) before computing absBaseDir and rel, ensuring
the overlap check that computes isInside (using absBaseDir, absOutputDir and
path.relative/path.isAbsolute) runs for every pattern rather than being gated on
baseDir truthiness.
8b8405d to
3e30b66
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/utils/src/wildcards.ts (1)
89-109: PreservepublicFolderwhen no wildcards are present.When
hasWildcardsis false, the code returns the originalinputarray, which drops thepublicFolderprefix. That breaks explicit paths inpublicFoldermode.🐛 Proposed fix
- const processedPaths = hasWildcards - ? fg.globSync(inputWithPublicFolder, { ignore: options.ignore }) - : input; + const processedPaths = hasWildcards + ? fg.globSync(inputWithPublicFolder, { ignore: options.ignore }) + : inputWithPublicFolder;
🤖 Fix all issues with AI agents
In `@packages/utils/src/wildcards.ts`:
- Around line 45-64: Update the JSDoc for the exported function wildcards to
include an `@returns` tag that describes the function's return value (it delegates
to wildcardsArray or wildcardsString and ultimately returns an array of resolved
file paths, i.e., string[]). Edit the comment block above the wildcards function
to add a concise `@returns` line explaining it returns an array of matched/real
file paths (string[]), and ensure the wording references that it handles single
string or array input and returns resolved paths from
wildcardsArray/wildcardsString.
🧹 Nitpick comments (1)
packages/action/src/benchmark.ts (1)
48-53: Minor inconsistency: Use destructuredinputin error message.For consistency with the rest of the function, use the destructured
inputvariable instead ofinputs.inputin the error message at line 50.♻️ Suggested fix
const fileResult = result.files[0]; if (!fileResult) { throw new Error( - `Benchmark failed: no results for input "${inputs.input}". ` + + `Benchmark failed: no results for input "${input}". ` + "Check that the file exists and at least one compressor succeeded." ); }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/utils/src/wildcards.ts`:
- Around line 102-104: The processedPaths assignment in the wildcards function
uses inputWithPublicFolder only when hasWildcards is true, which ignores
options.publicFolder for non-wildcard inputs; change the logic so processedPaths
is inputWithPublicFolder when hasWildcards is false as well (i.e., use
inputWithPublicFolder instead of input), or if backward-compatibility is
required, add a clear comment in the wildcards function explaining the
intentional behavior; locate the processedPaths variable and
inputWithPublicFolder/hasWildcards references to implement the change or add the
explanatory comment.
|
@greptile |
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
/improve |
|
Persistent suggestions updated to latest commit a8273bb |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/action/src/index.ts`:
- Around line 148-151: Dry-run branch returns early without setting outputs,
unlike the empty-files branch; update the inputs.dryRun path so it sets the same
default outputs as the files.length === 0 branch before returning. Concretely,
locate the output-setting logic used when no files are found and invoke that
same logic (or duplicate the same setOutput calls) from the if (inputs.dryRun)
block right after the info(...) call so downstream consumers receive the
expected outputs.
♻️ Duplicate comments (1)
packages/action/src/validate.ts (1)
9-17: Add@returnstag to JSDoc for exported function.Per coding guidelines, all exported functions must have JSDoc with
@paramand@returnstags. Since this function returnsvoid, add@returns voidor@returns {void} Nothing - throws on validation failure.📝 Proposed fix
/** * Validates that the output directory is not inside any source pattern. * Prevents infinite loop risk when output overlaps with source files. * * `@param` outputDir - The output directory path * `@param` patterns - Array of source file patterns (glob patterns) * `@param` workingDirectory - Base directory for resolving relative paths (defaults to ".") + * `@returns` void * `@throws` Error if outputDir is inside any source pattern */
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/action/src/index.ts">
<violation number="1" location="packages/action/src/index.ts:126">
P2: Normalize the output-dir before building the ignore glob. `./` prefixes produce an ignore pattern that won’t match discovered relative paths, so outputs can be re-processed and re-minified in auto mode.</violation>
</file>
<file name="packages/action/src/validate.ts">
<violation number="1" location="packages/action/src/validate.ts:29">
P2: Root-recursive glob patterns (e.g., `**/*.js`) are skipped here because `baseDir` is empty, so `outputDir` can overlap the matched tree and create self-including minification loops. The skip should only apply to non-recursive root-level patterns.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/action/src/index.ts">
<violation number="1" location="packages/action/src/index.ts:128">
P2: When `output-dir` is "./", normalization strips it to an empty string and the generated ignore glob becomes `**//**`, which matches everything and causes auto mode to discover zero files. Preserve "." for an empty normalized output-dir to avoid ignoring the entire workspace.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
srod
force-pushed
the
feat/zero-config-action
branch
from
c4ec306 to
b1f6919
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/action/__tests__/validate.test.ts`:
- Around line 49-53: The test uses "../outside" which is rejected earlier by
parseInputs (segments.includes("..")) so it never exercises validateOutputDir in
realistic flow; update the test to use a path that would pass parseInputs (e.g.,
"outside" or "./outside") so validateOutputDir is actually reachable, or add a
comment clarifying the test is intentionally isolating validateOutputDir; target
symbols: parseInputs and validateOutputDir.
In `@packages/utils/__tests__/wildcards-ignore.test.ts`:
- Around line 9-10: Update the two imports that reference "../src/wildcards.js"
to use the TypeScript module extension ".ts"; specifically change the import of
the type WildcardOptions and the values DEFAULT_IGNORES and wildcards so they
import from "../src/wildcards.ts" (ensure both import statements are updated to
reference WildcardOptions, DEFAULT_IGNORES, and wildcards from the .ts path).
🧹 Nitpick comments (12)
packages/utils/__tests__/wildcards-ignore.test.ts (1)
101-113: Test name is misleading.The test is named "should use DEFAULT_IGNORES when no ignore option provided" but actually tests explicit usage of
DEFAULT_IGNORES. The comment on line 104 acknowledges this ("Test that we can use DEFAULT_IGNORES explicitly"). The implementation doesn't automatically applyDEFAULT_IGNORESwhenignoreis omitted—it passesundefined.Consider renaming for clarity:
✏️ Suggested rename
- test("should use DEFAULT_IGNORES when no ignore option provided", () => { + test("should work when DEFAULT_IGNORES is explicitly provided as ignore option", () => { vi.mocked(fg.globSync).mockReturnValue(["app.js"]); - // Test that we can use DEFAULT_IGNORES explicitly const options: WildcardOptions = { ignore: DEFAULT_IGNORES, };packages/utils/src/wildcards.ts (1)
51-54: Consider adding an explicit return type for API clarity.The function's return type is inferred as
{} | { input: string[] }. Adding an explicit return type would improve IDE support and make the API contract clearer.✏️ Suggested type annotation
+interface WildcardResult { + input?: string[]; +} + export function wildcards( input: string | string[], options?: WildcardOptions | string -) { +): WildcardResult {packages/action/src/benchmark.ts (1)
50-56: Minor inconsistency in error message reference.Line 50 still uses
inputs.inputin the error message while line 56 uses the destructuredinput. Both reference the same value, so this works correctly, but consider using the destructuredinputconsistently for clarity.♻️ Suggested consistency fix
if (!fileResult) { throw new Error( - `Benchmark failed: no results for input "${inputs.input}". ` + + `Benchmark failed: no results for input "${input}". ` + "Check that the file exists and at least one compressor succeeded." ); }packages/action/src/discover.ts (1)
18-35: Consider reusingDEFAULT_IGNORESfrom@node-minify/utils.The
DEFAULT_IGNORESarray here is identical to the one exported frompackages/utils/src/wildcards.ts. Importing it would avoid duplication and ensure consistency if the ignore patterns are updated in the future.♻️ Proposed refactor
import path from "node:path"; import * as core from "@actions/core"; import fg from "fast-glob"; +import { DEFAULT_IGNORES } from "@node-minify/utils"; // ... keep DEFAULT_PATTERNS as-is ... -export const DEFAULT_IGNORES = [ - "**/node_modules/**", - "**/dist/**", - "**/build/**", - "**/.next/**", - "**/*.min.{js,css}", - "**/*.d.ts", - "**/__tests__/**", - "**/.*", -]; +export { DEFAULT_IGNORES };packages/action/__tests__/index.test.ts (1)
11-48: Consider adding tests for invalidsizeparameter.The
chunkArrayimplementation (per the relevant code snippet) throws aTypeErrorfor non-positive or non-integer sizes. Adding tests for these error cases would improve coverage.🧪 Suggested additional tests
test("throws TypeError for size of 0", () => { expect(() => chunkArray([1, 2, 3], 0)).toThrow(TypeError); }); test("throws TypeError for negative size", () => { expect(() => chunkArray([1, 2, 3], -1)).toThrow(TypeError); }); test("throws TypeError for non-integer size", () => { expect(() => chunkArray([1, 2, 3], 2.5)).toThrow(TypeError); });packages/action/__tests__/inputs.test.ts (1)
269-409: Consider adding tests foroutput-dirsecurity validation.The auto mode tests cover the main functionality well. However, based on the implementation in
inputs.ts(lines 63-67),output-dirrejects absolute paths and..segments. Consider adding tests to verify this security hardening:💡 Suggested additional tests
test("throws error for absolute output-dir", () => { vi.mocked(getInput).mockImplementation((name: string) => { if (name === "output-dir") return "/absolute/path"; return ""; }); vi.mocked(getBooleanInput).mockImplementation((name: string) => { if (name === "auto") return true; return false; }); expect(() => parseInputs()).toThrow( 'output-dir must be a relative path without ".." segments' ); }); test("throws error for output-dir with path traversal", () => { vi.mocked(getInput).mockImplementation((name: string) => { if (name === "output-dir") return "dist/../../../etc"; return ""; }); vi.mocked(getBooleanInput).mockImplementation((name: string) => { if (name === "auto") return true; return false; }); expect(() => parseInputs()).toThrow( 'output-dir must be a relative path without ".." segments' ); });packages/action/src/reporters/summary.ts (1)
136-156: Side effect inmapcallback accumulates totals.The
totalOriginalandtotalMinifiedaccumulation inside themapcallback works correctly but mixes iteration concerns. This is a minor style observation—the current implementation is readable and functional.♻️ Optional: Separate accumulation from row building
+ // Calculate type totals + for (const f of files) { + totalOriginal += f.originalSize; + totalMinified += f.minifiedSize; + } + const rows = files.map((f) => { - totalOriginal += f.originalSize; - totalMinified += f.minifiedSize; - const row = [ { data: `\`${f.file}\`` }, { data: prettyBytes(f.originalSize) },packages/action/__tests__/discover.test.ts (1)
1-11: Missing copyright header.Per coding guidelines, source files should include a copyright header. While this is a test file, the other test files in this PR include a header like
/*! node-minify action tests - MIT Licensed */.💡 Suggested fix
+/*! node-minify action tests - MIT Licensed */ + import path from "node:path"; import * as core from "@actions/core";packages/action/src/index.ts (4)
55-61: Consider enhancing the JSDoc for consistency.While
getFileSizeis not exported (so the strict JSDoc requirement doesn't apply), adding@paramand@returnstags would improve consistency with other functions in this file.📝 Suggested JSDoc enhancement
/** * Get the size of a file in bytes. + * + * `@param` filePath - Path to the file + * `@returns` The file size in bytes */ async function getFileSize(filePath: string): Promise<number> {
300-312: Auto mode lacks PR comment and annotation support.Unlike
runExplicitMode(lines 82-89),runAutoModedoesn't supportreportPRCommentorreportAnnotationsinputs. If this is intentional for the initial implementation, consider documenting this limitation. Otherwise, users may expect feature parity between modes.📝 Optional: Add PR comment and annotation support for parity
if (inputs.reportSummary) { await generateSummary(minifyResult); } + + if (inputs.reportPRComment && context.payload.pull_request) { + const comparisons = await compareWithBase(minifyResult, inputs.githubToken); + await postPRComment(minifyResult, inputs.githubToken, comparisons); + } + + if (inputs.reportAnnotations) { + addAnnotations(minifyResult); + } const thresholdError = checkThresholds(minifyResult.totalReduction, inputs);
347-347: Consider handling the promise rejection at the entry point.The
run()invocation doesn't handle potential unhandled promise rejections. While the internal error handling callssetFailed, an unhandled rejection could still occur if something unexpected throws.📝 Suggested enhancement
-run(); +run().catch((error) => { + setFailed(error instanceof Error ? error.message : "Unhandled error in run()"); +});
201-205: Remove the redundantas FileTypecast.
detectFileType(file)already returns typeFileTypeexplicitly (never null or unknown value), andCOMPRESSOR_MAPprovides entries for allFileTypevariants including"unknown". The cast is unnecessary.
| test("does not throw when output is outside using relative path traversal", () => { | ||
| expect(() => { | ||
| validateOutputDir("../outside", ["src/**"]); | ||
| }).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find packages/action/src -name "*.ts" | head -20Repository: srod/node-minify
Length of output: 538
🏁 Script executed:
rg -n "segments.includes\|segments\.some\|contains.*\.\." packages/action/src/ -A3 -B3Repository: srod/node-minify
Length of output: 42
🏁 Script executed:
rg -n "validateOutputDir\|parseInputs" packages/action/src/inputs.ts -A5 -B2Repository: srod/node-minify
Length of output: 42
🏁 Script executed:
cat -n packages/action/src/inputs.ts | head -80Repository: srod/node-minify
Length of output: 3790
🏁 Script executed:
cat -n packages/action/src/validate.ts | head -100Repository: srod/node-minify
Length of output: 2218
🏁 Script executed:
cat -n packages/action/__tests__/validate.test.ts | sed -n '40,60p'Repository: srod/node-minify
Length of output: 878
🏁 Script executed:
rg -n "validateOutputDir" packages/action/src/inputs.ts -A5 -B5Repository: srod/node-minify
Length of output: 936
Test case documents unreachable code path: ../outside is rejected before reaching validateOutputDir.
The test correctly asserts that validateOutputDir itself does not reject ../ segments—it only validates that output doesn't overlap with source patterns. However, in actual runtime flow, parseInputs rejects ../outside at line 65 (segments.includes("..")) before calling validateOutputDir at line 86. This means the test documents validateOutputDir's behavior in isolation, but the specific test case (../outside) will never actually reach validateOutputDir in production.
Either document that this test only validates validateOutputDir in isolation, or adjust the test to use a path that would realistically pass the .. check but still be relevant to validateOutputDir's validation logic.
🤖 Prompt for AI Agents
In `@packages/action/__tests__/validate.test.ts` around lines 49 - 53, The test
uses "../outside" which is rejected earlier by parseInputs
(segments.includes("..")) so it never exercises validateOutputDir in realistic
flow; update the test to use a path that would pass parseInputs (e.g., "outside"
or "./outside") so validateOutputDir is actually reachable, or add a comment
clarifying the test is intentionally isolating validateOutputDir; target
symbols: parseInputs and validateOutputDir.
Adds zero-config auto mode to the GitHub Action enabling automatic file discovery and minification without explicit input/output configuration. Features: - Auto mode (auto: true) discovers source files using smart default patterns - File type detection for JS, CSS, HTML, JSON, SVG with compressor selection - Parallel processing (4 concurrent) for efficient batch minification - Output directory structure preservation - Dry-run preview mode with consistent output handling New inputs: auto, patterns, output-dir, ignore, dry-run Security hardening: - Path traversal prevention with segment-based validation - Absolute path rejection for output-dir - Output directory overlap validation with source patterns - Input sanitization in generateOutputPath() - Auto-ignore outputDir during discovery (prevents re-processing) - Normalize outputDir before building ignore glob - Handle empty outputDir edge case Also includes: - @node-minify/utils: Add ignore patterns support to wildcards() - @node-minify/utils: Fix publicFolder handling for non-wildcard paths - @node-minify/utils: Add @returns JSDoc tag to wildcards()
Unit tests (176 total, 85%+ coverage): - autoDetect.test.ts: file type detection, compressor selection - discover.test.ts: file discovery, output path generation, security - validate.test.ts: output directory validation - inputs.test.ts: auto mode input parsing - index.test.ts: run() entry point routing - runAutoMode.test.ts: auto mode orchestration - runExplicitMode.test.ts: explicit mode orchestration - minify.test.ts: runMinification wrapper - summary.test.ts: auto mode summary reporter - wildcards-ignore.test.ts: ignore patterns support CI integration tests: - Zero Config Auto Mode - Zero Config Dry Run - Zero Config Custom Patterns
srod
force-pushed
the
feat/zero-config-action
branch
from
b1f6919 to
d02b331
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/action/src/benchmark.ts (1)
48-53: Minor inconsistency: error message usesinputs.inputinstead of destructuredinput.For consistency with the rest of the function, the error message should use the destructured
inputvariable.Proposed fix
const fileResult = result.files[0]; if (!fileResult) { throw new Error( - `Benchmark failed: no results for input "${inputs.input}". ` + + `Benchmark failed: no results for input "${input}". ` + "Check that the file exists and at least one compressor succeeded." ); }packages/action/README.md (1)
96-109: Document the new auto-mode inputs.The inputs table is missing the five new auto-mode parameters mentioned in the PR objectives:
auto,patterns,output-dir,ignore, anddry-run. Users need this reference documentation to understand available options.Additionally,
inputandoutputare marked as "Required: Yes", but they should be optional whenauto: true.📝 Suggest adding auto-mode inputs to the table
Add the following rows to the inputs table:
| `input` | Files to minify (glob pattern or path) | Yes | - | | `output` | Output file path | Yes | - | +| `auto` | Enable zero-config auto discovery | No | `false` | +| `patterns` | Custom glob patterns (comma-separated) | No | Smart defaults | +| `output-dir` | Output directory for auto mode | No | `dist` | +| `ignore` | Patterns to ignore (comma-separated) | No | `node_modules`, `dist`, etc. | +| `dry-run` | Preview changes without writing files | No | `false` | | `compressor` | Compressor to use | No | `terser` |And update the required column for
inputandoutput:-| `input` | Files to minify (glob pattern or path) | Yes | - | -| `output` | Output file path | Yes | - | +| `input` | Files to minify (glob pattern or path) | Yes* | - | +| `output` | Output file path | Yes* | - |With a note:
*Required unlessauto: trueis set.
🤖 Fix all issues with AI agents
In `@packages/action/README.md`:
- Around line 53-54: The "Install compressors" workflow example only installs
`@node-minify/terser` and `@node-minify/lightningcss` which will break auto mode for
HTML, JSON, and SVG files; update the README's example step named "Install
compressors" to include `@node-minify/html-minifier`, `@node-minify/jsonminify`, and
`@node-minify/svgo` OR add a short note next to that step explaining that users
must install the HTML/JSON/SVG compressors when their repo contains
.html/.json/.svg files so auto mode won't error.
♻️ Duplicate comments (2)
packages/action/src/validate.ts (2)
9-17: Add@returnstag to exported function JSDoc.The JSDoc is missing a
@returnstag. Per coding guidelines, all exported functions must have JSDoc comments with@paramand@returnstags.📝 Suggested fix
/** * Validates that the output directory is not inside any source pattern. * Prevents infinite loop risk when output overlaps with source files. * * `@param` outputDir - The output directory path * `@param` patterns - Array of source file patterns (glob patterns) * `@param` workingDirectory - Base directory for resolving relative paths (defaults to ".") + * `@returns` void * `@throws` Error if outputDir is inside any source pattern */
29-33: Root-recursive glob patterns (**/) bypass overlap validation.Patterns starting with
**/(e.g.,**/*.js) result in an emptybaseDiraftersplit("*")[0], causing the validation to be skipped entirely. This allowsoutputDirto overlap with matched files, defeating the infinite-loop prevention this function is meant to provide.Consider treating empty
baseDirfrom recursive patterns as the current working directory:🔧 Suggested fix
// Extract base directory from glob pattern (everything before first *) const baseDir = pattern.split("*")[0]; - // Skip root-level patterns (e.g., "*.js") - they don't recurse into subdirs - // so outputDir in a subdir like "dist" won't cause infinite loops - if (!baseDir || baseDir === "") { + // For root-level non-recursive patterns (e.g., "*.js"), skip validation + // as they don't recurse into subdirs like "dist" + const isRootRecursive = pattern.startsWith("**/") || pattern.startsWith("**\\"); + if (!baseDir && !isRootRecursive) { continue; } - const absBaseDir = path.resolve(workingDirectory, baseDir); + // For recursive patterns, use working directory as base + const effectiveBaseDir = baseDir || "."; + const absBaseDir = path.resolve(workingDirectory, effectiveBaseDir);
🧹 Nitpick comments (6)
packages/action/src/reporters/summary.ts (1)
136-156: Consider separating accumulation from row mapping.Accumulating
totalOriginalandtotalMinifiedinside themapcallback mixes side effects with transformation, which can be harder to reason about.♻️ Suggested refactor
+ // Calculate totals for this group + for (const f of files) { + totalOriginal += f.originalSize; + totalMinified += f.minifiedSize; + } + const rows = files.map((f) => { - totalOriginal += f.originalSize; - totalMinified += f.minifiedSize; - const row = [ { data: `\`${f.file}\`` }, { data: prettyBytes(f.originalSize) },packages/action/__tests__/runAutoMode.test.ts (1)
1-1: Copyright header does not match the standard format.The header should follow the standard format used in source files. Per coding guidelines, source files should include the copyright header
/*! node-minify ... MIT Licensed */.📝 Suggested fix
-/*! node-minify action tests - MIT Licensed */ +/*! + * node-minify + * Copyright (c) 2011-2026 Rodolphe Stoclin + * MIT Licensed + */packages/action/__tests__/discover.test.ts (1)
1-4: Add standard copyright header to test file.The test file is missing the standard copyright header that should be present in all source files.
📝 Suggested fix
+/*! + * node-minify + * Copyright (c) 2011-2026 Rodolphe Stoclin + * MIT Licensed + */ + import path from "node:path"; import * as core from "@actions/core"; import fg from "fast-glob"; import { afterEach, describe, expect, test, vi } from "vitest";packages/action/src/discover.ts (1)
26-35:DEFAULT_IGNORESduplicates definition in@node-minify/utils.This constant is identical to
DEFAULT_IGNORESexported frompackages/utils/src/wildcards.ts. Consider importing it from@node-minify/utilsto maintain a single source of truth and avoid divergence.📝 Suggested refactor
+import { DEFAULT_IGNORES } from "@node-minify/utils"; - -export const DEFAULT_IGNORES = [ - "**/node_modules/**", - "**/dist/**", - "**/build/**", - "**/.next/**", - "**/*.min.{js,css}", - "**/*.d.ts", - "**/__tests__/**", - "**/.*", -]; + +export { DEFAULT_IGNORES };packages/action/src/index.ts (1)
201-205: Remove redundant type cast.
detectFileType(file)already returnsFileType, so the castfileType as FileTypeis unnecessary on line 204.📝 Suggested fix
const fileType = detectFileType(file); - const { compressor, type } = selectCompressor( - fileType as FileType - ); + const { compressor, type } = selectCompressor(fileType);packages/utils/__tests__/wildcards-ignore.test.ts (1)
101-107: Align the test name with what it asserts.This test passes
DEFAULT_IGNORESexplicitly, so “no ignore option” is misleading.✏️ Rename suggestion
- test("should use DEFAULT_IGNORES when no ignore option provided", () => { + test("should pass DEFAULT_IGNORES when provided", () => {
| - name: Install compressors | ||
| run: npm install @node-minify/terser @node-minify/lightningcss |
There was a problem hiding this comment.
Incomplete compressor installation may cause runtime errors.
The example installs only @node-minify/terser and @node-minify/lightningcss, but auto mode supports five file types. Users with HTML, JSON, or SVG files will encounter errors without @node-minify/html-minifier, @node-minify/jsonminify, and @node-minify/svgo.
📦 Suggest installing all supported compressors or adding a note
Option 1: Install all compressors in the example:
- name: Install compressors
- run: npm install `@node-minify/terser` `@node-minify/lightningcss`
+ run: npm install `@node-minify/terser` `@node-minify/lightningcss` `@node-minify/html-minifier` `@node-minify/jsonminify` `@node-minify/svgo`Option 2: Add a note explaining which compressors are needed:
- name: Install compressors
run: npm install `@node-minify/terser` `@node-minify/lightningcss`
+ # Install additional compressors as needed:
+ # `@node-minify/html-minifier` (HTML), `@node-minify/jsonminify` (JSON), `@node-minify/svgo` (SVG)📝 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.
| - name: Install compressors | |
| run: npm install @node-minify/terser @node-minify/lightningcss | |
| - name: Install compressors | |
| run: npm install `@node-minify/terser` `@node-minify/lightningcss` `@node-minify/html-minifier` `@node-minify/jsonminify` `@node-minify/svgo` |
| - name: Install compressors | |
| run: npm install @node-minify/terser @node-minify/lightningcss | |
| - name: Install compressors | |
| run: npm install `@node-minify/terser` `@node-minify/lightningcss` | |
| # Install additional compressors as needed: | |
| # `@node-minify/html-minifier` (HTML), `@node-minify/jsonminify` (JSON), `@node-minify/svgo` (SVG) |
🤖 Prompt for AI Agents
In `@packages/action/README.md` around lines 53 - 54, The "Install compressors"
workflow example only installs `@node-minify/terser` and `@node-minify/lightningcss`
which will break auto mode for HTML, JSON, and SVG files; update the README's
example step named "Install compressors" to include `@node-minify/html-minifier`,
`@node-minify/jsonminify`, and `@node-minify/svgo` OR add a short note next to that
step explaining that users must install the HTML/JSON/SVG compressors when their
repo contains .html/.json/.svg files so auto mode won't error.
- Updated docs/src/content/docs/github-action.md with zero-config section - Updated packages/action/README.md with usage examples and installation notes - Added changeset for @node-minify/action and @node-minify/utils minor bump - Added .sisyphus and .claude to .gitignore
srod
force-pushed
the
feat/zero-config-action
branch
from
7955b14 to
c21e335
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2797 +/- ##
===========================================
- Coverage 99.40% 95.07% -4.33%
===========================================
Files 64 72 +8
Lines 1345 1686 +341
Branches 419 512 +93
===========================================
+ Hits 1337 1603 +266
- Misses 8 83 +75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
Adds zero-config auto mode to the GitHub Action, enabling automatic file discovery and minification without explicit input/output configuration.
What's New
Auto Mode (
auto: true)New Inputs
autofalsepatternsoutput-dirdistignoredry-runfalseSupported File Types
.js,.mjs,.cjs,.jsx@node-minify/terser.css@node-minify/lightningcss.html,.htm@node-minify/html-minifier.json@node-minify/jsonminify.svg@node-minify/svgoSmart Defaults
src/,app/,lib/,styles/, root-level filesnode_modules,dist,build,.next,*.min.*,*.d.ts,__tests__, dotfilesExample Usage
Security Hardening
This PR includes comprehensive security measures for path handling:
inputs.tsrejects absoluteoutput-dirpaths..detection (not substring matching)path.resolve()+path.relative()to prevent source overlapgenerateOutputPath()normalizes and strips traversal sequencesTesting
New Test Files
runAutoMode.test.tsrunExplicitMode.test.tsminify.test.tsindex.test.tsdiscover.test.tsvalidate.test.tsCode Review Feedback Addressed
Greptile
detectFileType()utilityCodeRabbit
generateOutputPathagainst absolute paths..traversal escaping output directory@returnsJSDoc tags to exported functionsCubic
validateOutputDirusingpath.resolve()+path.relative()Documentation
docs/src/content/docs/github-action.mdpackages/action/README.mdBreaking Changes
None. This is a backward-compatible addition. Explicit mode continues to work as before.
Verification Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores / Tests / CI
✏️ Tip: You can customize this high-level summary in your review settings.