Conversation
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
fix(deps): update dependency oxc-minify to ^0.108.0
- Add new packages/action with Node20 runtime for GitHub Marketplace - Includes minification, PR comments, job summaries, annotations, benchmarking - Supports 22 compressors with deprecation warnings for legacy ones - Add getFilesizeGzippedRaw to utils for raw byte gzip sizes - Add minify-html to compressor registry - Add oxc to default benchmark compressors - Update .github/actions/node-minify to use resolveCompressor from utils - Refactor action imports to use named imports instead of namespace imports
- Create root action.yml for uses: srod/node-minify@v1 syntax - Switch from ncc to Bun bundler for 673KB self-contained bundle - Add release-action.yml workflow for automated releases - Update test-action.yml to build and test the bundled action - Deprecate old composite action at .github/actions/node-minify - Add GitHub Action documentation page to Astro docs site - Add index.d.ts for type exports
The action build requires @node-minify/core and @node-minify/utils to be built first since Bun's bundler resolves workspace dependencies.
The action imports from @node-minify/core for the minify function.
The bundled action dynamically imports compressor packages at runtime, so they need to be installed in the test environment.
…ocol npm fails when run in repo root due to workspace:* dependencies in package.json.
Node's import() looks in the repo's node_modules, not in NODE_PATH.
Also copy the compressor's own dependencies (terser, esbuild, lightningcss).
Copying to root node_modules is sufficient as long as we copy ALL packages.
This prevents the 'No such file or directory' error while keeping the installation clean.
…FilesizeGzippedRaw chore(scripts): improve error handling in check-published
Docstrings generation was requested by @srod. * #2760 (comment) The following files were modified: * `packages/action/src/checks.ts` * `packages/action/src/index.ts` * `packages/action/src/inputs.ts` * `packages/action/src/minify.ts` * `packages/action/src/outputs.ts` * `packages/action/src/reporters/annotations.ts` * `packages/action/src/reporters/comment.ts` * `packages/action/src/reporters/summary.ts` * `packages/benchmark/src/runner.ts` * `packages/utils/src/getFilesizeGzippedInBytes.ts` * `scripts/check-published.ts`
…627895 🛡️ Sentinel: [Security Enhancement] Add silence option to run utility
- Update `Header.astro` to use `aria-label` for top navigation - Update `MainLayout.astro` to use `aria-label` for sidebar navigation - Update `LeftSidebar.astro` to use `aria-label` for main navigation - Add learning to `.Jules/palette.md`
…9028464055844 🎨 Palette: Use aria-label for navigation landmarks in docs
Optimizes `writeFile` and `writeFileAsync` in `packages/utils` by removing the explicit `lstat` check for directories before writing. Instead, it relies on the underlying `fs` write operation to throw `EISDIR` if the target is a directory. This saves one syscall per write operation. - Removes `isDirectory` and `isDirectoryAsync` checks. - Updates `writeFile` to catch filesystem errors naturally. - Maintains backward compatibility by ensuring errors are still wrapped in `FileOperationError` (via `handleWriteError`). - Verified with existing tests.
…786206695180100 ⚡ Bolt: Use EAFP pattern in writeFile to reduce syscalls
- Created `Pagination` component to generate Previous/Next links based on sidebar structure. - Integrated `Pagination` into `MainLayout` to appear at the bottom of content pages. - Added accessibility features (aria-labels, focus outlines, semantic HTML). - Styled with existing design system variables.
…732784843 🎨 Palette: Add pagination to documentation pages
fix(deps): update dependency ora to v9.1.0
Adds allowEmptyOutput option that gracefully handles empty minification results (e.g., CSS files with only comments) without throwing errors. - Add allowEmptyOutput to Settings type - Implement in writeOutput() to skip writing when content is empty - Add --allow-empty-output CLI flag - Add tests covering all scenarios - Add documentation Closes #2790
Commander 14.x requires explicit .action() on main program when subcommands exist, otherwise shows help instead of running. Also includes biome formatting fixes.
Docstrings generation was requested by @srod. * #2791 (comment) The following files were modified: * `packages/cli/src/bin/cli.ts` * `packages/cli/src/compress.ts` * `packages/google-closure-compiler/src/index.ts` * `packages/utils/src/run.ts`
📝 Add docstrings to `feature/issue-2790-allow-empty-output`
- cli.ts: treat empty input array as missing (defaults to []) - compress.ts: check file existence explicitly, let IO errors propagate - run.ts: only skip when ALL output channels empty (code, buffer, outputs)
feat(utils): add allowEmptyOutput option + fix CLI Commander 14.x
fix(deps): update dependency oxc-minify to ^0.110.0
🦋 Changeset detectedLatest commit: cc5fd76 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. 📝 WalkthroughWalkthroughIntroduces a new, bundled GitHub Action package ( Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant GHA as GitHub Actions
participant Inputs as Input Parser (`packages/action/src/inputs.ts`)
participant Minifier as Minifier (`packages/action/src/minify.ts`)
participant Benchmark as Benchmark (`packages/action/src/benchmark.ts`)
participant Reporter as Reporters (summary/comment/annotations)
participant Threshold as Checks (`packages/action/src/checks.ts`)
User->>GHA: Trigger action (workflow)
GHA->>Inputs: parseInputs()
Inputs-->>GHA: ActionInputs
GHA->>Minifier: runMinification(ActionInputs)
Minifier->>Minifier: resolveCompressor (utils)
Minifier->>Minifier: perform minify, measure sizes/time
Minifier-->>GHA: MinifyResult
alt benchmark enabled
GHA->>Benchmark: runBenchmark(ActionInputs)
Benchmark->>Benchmark: run iterations across compressors
Benchmark-->>GHA: BenchmarkResult
GHA->>Reporter: generateBenchmarkSummary(BenchmarkResult)
end
GHA->>Reporter: generateSummary(MinifyResult)
GHA->>Reporter: postPRComment(MinifyResult)
GHA->>Reporter: addAnnotations(MinifyResult)
GHA->>Threshold: checkThresholds(MinifyResult.totalReduction)
Threshold-->>GHA: pass | violation
GHA-->>User: setOutputs & final status (success/fail)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile Summary
Important Files Changed
Confidence score: 4/5
Sequence DiagramsequenceDiagram
participant User as "GitHub Workflow"
participant Action as "action/index.ts"
participant Inputs as "inputs.ts"
participant Minify as "minify.ts"
participant Core as "@node-minify/core"
participant Utils as "@node-minify/utils"
participant Outputs as "outputs.ts"
participant Summary as "reporters/summary.ts"
participant Comment as "reporters/comment.ts"
participant Annotations as "reporters/annotations.ts"
participant Checks as "checks.ts"
User->>Action: "Trigger workflow"
Action->>Inputs: "parseInputs()"
Inputs-->>Action: "ActionInputs"
Action->>Inputs: "validateCompressor(compressor)"
Action->>Minify: "runMinification(inputs)"
Minify->>Utils: "resolveCompressor(compressor)"
Utils-->>Minify: "compressor function"
Minify->>Minify: "getFileSize(inputPath)"
Minify->>Core: "minify()"
Core-->>Minify: "compressed output"
Minify->>Minify: "getFileSize(outputPath)"
Minify->>Utils: "getFilesizeGzippedRaw(outputPath)"
Utils-->>Minify: "gzip size"
Minify-->>Action: "MinifyResult"
Action->>Outputs: "setMinifyOutputs(result)"
Outputs->>Outputs: "setOutput()"
Action->>Summary: "generateSummary(result)"
Summary->>Summary: "summary.addTable()"
Action->>Comment: "postPRComment(result, token)"
Comment->>Comment: "octokit.rest.issues.createComment()"
Action->>Annotations: "addAnnotations(result)"
Annotations->>Annotations: "warning/error/notice()"
Action->>Checks: "checkThresholds(reduction, inputs)"
Checks-->>Action: "null or error message"
Action-->>User: "Success or failure"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * - packages with unpublished versions (updates needed) | ||
| * - or a confirmation that all packages are up to date | ||
| */ | ||
| async function main() { |
There was a problem hiding this comment.
style: Function declared as async but never uses await - can be simplified to synchronous
| async function main() { | |
| function main() { |
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: scripts/check-published.ts
Line: 72:72
Comment:
**style:** Function declared as async but never uses await - can be simplified to synchronous
```suggestion
function main() {
```
<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.| required: true | ||
| type: string | ||
|
|
||
| concurrency: release-${{ github.event.release.tag_name || github.event.inputs.tag || github.ref_name }} |
There was a problem hiding this comment.
logic: Concurrency key uses multiple fallback values but could create conflicts if both release and workflow_dispatch run simultaneously with the same version. Should the concurrency key differentiate between release and workflow_dispatch events to prevent potential race conditions?
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/release-action.yml
Line: 13:13
Comment:
**logic:** Concurrency key uses multiple fallback values but could create conflicts if both release and workflow_dispatch run simultaneously with the same version. Should the concurrency key differentiate between release and workflow_dispatch events to prevent potential race conditions?
How can I resolve this? If you propose a fix, please make it concise.| git tag -fa "$VERSION" -m "Release $VERSION with built dist" | ||
| git push origin "refs/tags/$VERSION" --force |
There was a problem hiding this comment.
style: Force pushing tags can be dangerous in collaborative environments. Is force pushing tags acceptable for this release workflow, or should there be additional safeguards?
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: .github/workflows/release-action.yml
Line: 105:106
Comment:
**style:** Force pushing tags can be dangerous in collaborative environments. Is force pushing tags acceptable for this release workflow, or should there be additional safeguards?
<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.| // tryResolveLocalFile throws for local paths that can't be loaded, | ||
| // so reaching here means it returned null (which shouldn't happen | ||
| // after the isLocalPath guard, but we handle it defensively) | ||
| } |
There was a problem hiding this comment.
logic: unreachable code - tryResolveLocalFile always throws for local paths that can't be loaded
| // tryResolveLocalFile throws for local paths that can't be loaded, | |
| // so reaching here means it returned null (which shouldn't happen | |
| // after the isLocalPath guard, but we handle it defensively) | |
| } | |
| // 3. Try as local file path (throws if file exists but has invalid exports) | |
| if (isLocalPath(name)) { | |
| return await tryResolveLocalFile(name); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/utils/src/compressor-resolver.ts
Line: 285:288
Comment:
**logic:** unreachable code - tryResolveLocalFile always throws for local paths that can't be loaded
```suggestion
// 3. Try as local file path (throws if file exists but has invalid exports)
if (isLocalPath(name)) {
return await tryResolveLocalFile(name);
}
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/google-closure-compiler/src/index.ts (1)
75-106: Array-valued flags (e.g.,externs) are silently filtered, andnullis incorrectly accepted.The
applyOptionsfunction filters out arrays and acceptsnullas an object, silently ignoring valid multi-value flags that Google Closure Compiler supports.However, the proposed fix in
applyOptionsalone is insufficient. Arrays are filtered at a second stage in the pipeline:toBuildArgsOptions(inpackages/utils/src/buildArgs.tslines 21–28) also explicitly rejects arrays by design. Additionally,buildArgsconverts values withString(value), which would incorrectly stringify arrays as comma-separated lists instead of repeating flags (e.g.,--externs file1.js --externs file2.js).A complete fix requires:
- Updating
applyOptionsto accept string arrays and excludenull- Updating
toBuildArgsOptionsto support string arrays- Updating
buildArgsto repeat flags for array values instead of stringifying themCurrent applyOptions issue
type Flags = { [key: string]: string | boolean | Record<string, unknown>; }; // ... accepts null and rejects arrays at line 98: (typeof value === "object" && !Array.isArray(value))
🤖 Fix all issues with AI agents
In `@packages/action/src/outputs.ts`:
- Around line 24-30: The reduce uses a falsy check (f.gzipSize || 0) which will
incorrectly coerce a genuine 0 gzipSize; update the aggregation in the block
that computes totalGzip (over result.files) to use nullish coalescing so it
treats undefined/null as 0 but preserves 0 (i.e., replace the fallback with
(f.gzipSize ?? 0)), leaving the surrounding conditional and the
setOutput("gzip-size", totalGzip) call unchanged.
In `@packages/action/src/reporters/comment.ts`:
- Around line 35-64: Wrap the entire Octokit comment flow in a try/catch so API
failures don't fail the action: enclose the calls that use octokit.paginate,
octokit.rest.issues.updateComment and octokit.rest.issues.createComment (the
block that finds existingComment and either updates or creates a comment, using
generateCommentBody(result)) inside a try, and in the catch log a warning (e.g.,
warn(...) or info(...) with a clear message and the caught error) instead of
rethrowing so the action continues on comment post/update failure (common with
read-only GITHUB_TOKEN on forked PRs).
In `@packages/action/src/reporters/summary.ts`:
- Around line 20-27: The gzip size check in the rows mapping uses a truthy test
so a value of 0 becomes "-"—update the mapping in the result.files .map (the
rows variable) to use a nullish check for f.gzipSize (e.g., test f.gzipSize !=
null or the nullish coalescing operator) and only call prettyBytes when a
numeric gzipSize exists; keep everything else the same so 0 displays as "0 B"
(or equivalent) instead of "-".
In `@packages/html-minifier/src/index.ts`:
- Around line 44-50: The thrown error message "html-minifier failed: empty
result" gets double-wrapped by wrapMinificationError; update the thrown Error so
it does not include the compressor name prefix (remove "html-minifier" from the
message) — e.g. throw new Error("failed: empty result" or "empty result") in the
same location so wrapMinificationError produces a single, correctly prefixed
message; ensure wrapMinificationError is still used in the catch block.
In `@scripts/check-published.ts`:
- Around line 34-60: The current try/catch wraps both the package existence
check and the version-specific lookup so when execFileSync for `npm view
${packageName}@${version}` throws it incorrectly sets `exists` to false; modify
the code to perform the existence check (calling `execFileSync` for `latest` and
inspecting `latest`) inside its own try/catch to set `exists`, then separately
run the version-specific `execFileSync` (assign to `specific`) inside its own
try/catch that only sets `publishedVersion` based on whether the call succeeds
and `specific` is non-empty; keep using the existing identifiers `execFileSync`,
`packageName`, `version`, `latest`, `specific`, `exists`, and `publishedVersion`
so a thrown error on the version lookup does not flip `exists`.
🧹 Nitpick comments (14)
packages/utils/__tests__/isValidFileAsync.test.ts (1)
14-15: Minor: Avoid redundantfileURLToPathcall.
fileURLToPath(import.meta.url)is invoked twice. Consider computing it once and reusing:♻️ Suggested refactor
-const __dirname = path.dirname(fileURLToPath(import.meta.url)); -const __filename = fileURLToPath(import.meta.url); +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename);packages/utils/src/writeFile.ts (1)
127-136: Unsafe type cast:error as Error.The cast on line 134 assumes the caught value is an
Error, but JavaScript can throw any value. Per coding guidelines, useinstanceof Errorchecks.♻️ Safer error normalization
function handleWriteError(error: unknown, file: string | string[]): never { if (error instanceof ValidationError) { throw error; } + const normalizedError = + error instanceof Error ? error : new Error(String(error)); throw new FileOperationError( "write to", typeof file === "string" ? file : "multiple files", - error as Error + normalizedError ); }.github/actions/node-minify/minify.ts (1)
53-61: Use shared file utilities for size/existence checks.
PrefergetFilesizeInBytesandisValidFileAsyncfrom@node-minify/utilsover localstat+existsSyncto keep behavior consistent with the rest of the repo and avoid re-implementing file checks. Based on learnings, ...♻️ Proposed refactor
-import { appendFileSync, existsSync } from "node:fs"; -import { stat } from "node:fs/promises"; +import { appendFileSync } from "node:fs"; import { resolve } from "node:path"; import { minify } from "@node-minify/core"; import { + getFilesizeInBytes, getFilesizeGzippedInBytes, + isValidFileAsync, resolveCompressor, } from "@node-minify/utils"; @@ -async function getFileSize(filePath: string): Promise<number> { - const stats = await stat(filePath); - return stats.size; -} +// getFilesizeInBytes and isValidFileAsync from `@node-minify/utils` @@ - if (!existsSync(inputPath)) { + if (!(await isValidFileAsync(inputPath))) { console.error(`::error::Input file not found: ${inputPath}`); process.exit(1); } @@ - const originalSize = await getFileSize(inputPath); + const originalSize = await getFilesizeInBytes(inputPath); @@ - const minifiedSize = await getFileSize(outputPath); + const minifiedSize = await getFilesizeInBytes(outputPath);Also applies to: 93-96, 99-100, 135-136
.github/workflows/publish.yml (1)
30-30: Align Bun version with rootpackageManagerto avoid toolchain drift.CI pins bun-version to 1.3.6 while the repo specifies packageManager "bun@1.3.5" in package.json. Either bump the packageManager field to 1.3.6 or document the intentional mismatch to maintain consistency across local and CI environments.
packages/terser/package.json (1)
56-56: Exact pinning for stable terser 5.x is intentional and valid.The terser dependency uses exact pinning (
5.46.0) while some dependencies use caret ranges. However, this is a reasonable choice for stable versions: exact pinning prevents unexpected updates while caret ranges are more common for pre-release versions like^0.110.0(oxc-minify). Terser 5.46.0 contains only minor additions (DOM properties) with no breaking changes. If you prefer consistency with other dependencies that use caret ranges, you can switch to^5.46.0, but this is optional.packages/utils/src/getFilesizeBrotliInBytes.ts (1)
25-50: Consider using utils file I/O helpers for consistency. This module still usesexistsSyncandnode:fs/promisesdirectly; aligning with the shared file utilities would keep error handling consistent across the utils package. As per coding guidelines, please align with the@node-minify/utilsfile helpers where feasible.Also applies to: 58-96
packages/minify-html/src/index.ts (1)
41-53: Add an explicitErrorguard in the catch block.
Guidelines call forif (error instanceof Error)before wrapping unknown throws; this also preserves a proper cause when non-Error values are thrown.♻️ Proposed fix
} catch (error) { - throw wrapMinificationError("minify-html", error); + if (error instanceof Error) { + throw wrapMinificationError("minify-html", error); + } + throw wrapMinificationError("minify-html", new Error(String(error))); }As per coding guidelines, please add the
instanceof Errorcheck.packages/cssnano/src/index.ts (1)
30-39: Add an explicitErrorguard in the catch block.
This keeps error handling consistent with the repo convention and preserves cause data when non-Error values are thrown.♻️ Proposed fix
} catch (error) { - throw wrapMinificationError("cssnano", error); + if (error instanceof Error) { + throw wrapMinificationError("cssnano", error); + } + throw wrapMinificationError("cssnano", new Error(String(error))); }As per coding guidelines, please add the
instanceof Errorcheck.packages/cssnano/__tests__/cssnano-error.test.ts (1)
9-32: Align compressor tests with shared fixtures helpers.
This suite lives underpackages/cssnano/__tests__, where the convention istests/fixtures.tswithrunOneTest/commoncss; consider refactoring or briefly noting why this custom mock-based test is needed. As per coding guidelines, please align the test style with the shared fixtures convention.packages/utils/src/getFilesizeGzippedInBytes.ts (1)
20-40: Prefer async utils over sync fs checks ingetGzipSize.This helper is async but still uses
existsSync/isValidFile(sync). Consider aligning with the repo’s async file utilities to avoid sync I/O and double stat calls. As per coding guidelines, use the utils file operations where possible.♻️ Suggested refactor
-import { existsSync } from "node:fs"; -import { isValidFile } from "./isValidFile.ts"; +import { isValidFileAsync } from "./isValidFileAsync.ts"; +import { readFileAsync } from "./readFile.ts"; async function getGzipSize(file: string): Promise<number> { - if (!existsSync(file)) { - throw new FileOperationError( - "access", - file, - new Error("File does not exist") - ); - } - - if (!isValidFile(file)) { - throw new FileOperationError( - "access", - file, - new Error("Path is not a valid file") - ); - } + const isValid = await isValidFileAsync(file); + if (!isValid) { + throw new FileOperationError( + "access", + file, + new Error("Path is not a valid file") + ); + } const { gzipSize } = await import("gzip-size"); - const { readFile } = await import("node:fs/promises"); - const content = await readFile(file); + const content = await readFileAsync(file, true); return gzipSize(content); }.github/workflows/release-action.yml (1)
100-116: Consider adding error handling for the release tag push.The major version tag push (lines 113-116) has explicit error handling, but the release tag push at line 106 does not. If the force push fails, the workflow continues silently to the major tag step.
♻️ Suggested improvement
- name: Update release tag if: steps.commit.outputs.skip_tag_update != 'true' run: | # Update the release tag to point at HEAD (with dist files) and push VERSION="${{ steps.version.outputs.version }}" git tag -fa "$VERSION" -m "Release $VERSION with built dist" - git push origin "refs/tags/$VERSION" --force + if ! git push origin "refs/tags/$VERSION" --force; then + echo "Error: Failed to push release tag $VERSION" + exit 1 + fi.github/workflows/test-action.yml (1)
67-78: Compressor installation approach works but could be simplified.The current approach creates a temporary directory, installs packages there, then copies to workspace. This works around the monorepo structure but the copy step could fail silently if paths don't exist.
♻️ Consider adding verification
# Create node_modules and copy packages mkdir -p $GITHUB_WORKSPACE/node_modules cp -r /tmp/compressors/node_modules/* $GITHUB_WORKSPACE/node_modules/ + + # Verify installation + ls -la $GITHUB_WORKSPACE/node_modules/@node-minify/terser || exit 1 + ls -la $GITHUB_WORKSPACE/node_modules/@node-minify/esbuild || exit 1packages/action/src/benchmark.ts (1)
26-31: Consider validating that compressors list is not empty after filtering.If all compressors in
benchmarkCompressorsrequire a type andtypeis not provided, the filteredcompressorsarray will be empty. This could lead to a confusing error from the benchmark package rather than a clear action-specific message.♻️ Suggested improvement
// Filter out compressors that require 'type' when type is not provided const compressors = inputs.type ? inputs.benchmarkCompressors : inputs.benchmarkCompressors.filter( (c) => !TYPE_REQUIRED_COMPRESSORS.includes(c) ); + + if (compressors.length === 0) { + throw new Error( + `No compressors available for benchmark. ` + + `All specified compressors require a 'type' option. ` + + `Either specify 'type' or include compressors that don't require it.` + ); + }packages/action/src/minify.ts (1)
7-22: Prefer shared file utilities over direct fs stat.
UsegetFilesizeInBytesfrom@node-minify/utilsfor consistency and error handling.♻️ Suggested refactor
-import { stat } from "node:fs/promises"; import { resolve } from "node:path"; import { minify } from "@node-minify/core"; -import { getFilesizeGzippedRaw, resolveCompressor } from "@node-minify/utils"; +import { + getFilesizeGzippedRaw, + getFilesizeInBytes, + resolveCompressor, +} from "@node-minify/utils"; import type { ActionInputs, FileResult, MinifyResult } from "./types.ts"; -/** - * 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> { - const stats = await stat(filePath); - return stats.size; -} - @@ - const originalSize = await getFileSize(inputPath); + const originalSize = await getFilesizeInBytes(inputPath); @@ - const minifiedSize = await getFileSize(outputPath); + const minifiedSize = await getFilesizeInBytes(outputPath);As per coding guidelines, prefer shared file operation utilities.
Also applies to: 45-63
| if (typeof code !== "string") { | ||
| throw new Error("html-minifier failed: empty result"); | ||
| } | ||
|
|
||
| return { code }; | ||
| return { code }; | ||
| } catch (error) { | ||
| throw wrapMinificationError("html-minifier", error); |
There was a problem hiding this comment.
Potential double-wrapped error message.
The error thrown at line 45 ("html-minifier failed: empty result") will be caught by the outer catch block and re-wrapped via wrapMinificationError, resulting in: "html-minifier minification failed: html-minifier failed: empty result".
Consider using a message that doesn't include the compressor name prefix since wrapMinificationError will add it:
Proposed fix
if (typeof code !== "string") {
- throw new Error("html-minifier failed: empty result");
+ throw new Error("empty result");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (typeof code !== "string") { | |
| throw new Error("html-minifier failed: empty result"); | |
| } | |
| return { code }; | |
| return { code }; | |
| } catch (error) { | |
| throw wrapMinificationError("html-minifier", error); | |
| if (typeof code !== "string") { | |
| throw new Error("empty result"); | |
| } | |
| return { code }; | |
| } catch (error) { | |
| throw wrapMinificationError("html-minifier", error); |
🤖 Prompt for AI Agents
In `@packages/html-minifier/src/index.ts` around lines 44 - 50, The thrown error
message "html-minifier failed: empty result" gets double-wrapped by
wrapMinificationError; update the thrown Error so it does not include the
compressor name prefix (remove "html-minifier" from the message) — e.g. throw
new Error("failed: empty result" or "empty result") in the same location so
wrapMinificationError produces a single, correctly prefixed message; ensure
wrapMinificationError is still used in the catch block.
| try { | ||
| const latest = execFileSync( | ||
| "npm", | ||
| ["view", packageName, "version", "--json"], | ||
| { | ||
| stdio: "pipe", | ||
| } | ||
| ) | ||
| .toString() | ||
| .trim(); | ||
| if (latest === "" || latest === "undefined") | ||
| return { exists: false, publishedVersion: false }; | ||
|
|
||
| const specific = execFileSync( | ||
| "npm", | ||
| ["view", `${packageName}@${version}`, "version", "--json"], | ||
| { stdio: "pipe" } | ||
| ) | ||
| .toString() | ||
| .trim(); | ||
| return { | ||
| exists: true, | ||
| publishedVersion: specific !== "" && specific !== "undefined", | ||
| }; | ||
| } catch (_e) { | ||
| return { exists: false, publishedVersion: false }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file to see the current state
head -100 scripts/check-published.tsRepository: srod/node-minify
Length of output: 3572
🏁 Script executed:
# Also check if the file exists and its size
ls -lh scripts/check-published.ts 2>&1 && wc -l scripts/check-published.tsRepository: srod/node-minify
Length of output: 166
🏁 Script executed:
# Test the npm CLI behavior described in the review
#!/bin/bash
set -euo pipefail
pkg="@node-minify/core"
missing_version="0.0.0"
echo "=== Testing existing package ==="
npm view "$pkg" version --json 2>&1 | head -5
echo ""
echo "=== Testing non-existent version of existing package ==="
set +e
output=$(npm view "${pkg}@${missing_version}" version --json 2>&1)
exit_code=$?
echo "Output: $output"
echo "Exit code: $exit_code"
set -eRepository: srod/node-minify
Length of output: 803
Fix misclassification when a specific version lookup fails. When npm view ${packageName}@${version} throws (e.g., version does not exist), the current catch block returns { exists: false, publishedVersion: false }, incorrectly marking existing packages with unpublished versions as non-existent. This will cause existing packages with version mismatches to be placed in the "NOT FOUND" bucket instead of "UNPUBLISHED VERSIONS."
Separate the try/catch blocks: wrap the package existence check and version-specific check independently so that a thrown error on the version query only affects the publishedVersion flag, not the exists flag.
🔧 Proposed fix (separate version lookup)
function checkPublished(packageName: string, version: string) {
// Basic validation to prevent command injection
if (!/^[\w@/.-]+$/.test(packageName) || !/^[\w.-]+$/.test(version)) {
return { exists: false, publishedVersion: false };
}
- try {
- const latest = execFileSync(
- "npm",
- ["view", packageName, "version", "--json"],
- {
- stdio: "pipe",
- }
- )
- .toString()
- .trim();
- if (latest === "" || latest === "undefined")
- return { exists: false, publishedVersion: false };
-
- const specific = execFileSync(
- "npm",
- ["view", `${packageName}@${version}`, "version", "--json"],
- { stdio: "pipe" }
- )
- .toString()
- .trim();
- return {
- exists: true,
- publishedVersion: specific !== "" && specific !== "undefined",
- };
- } catch (_e) {
- return { exists: false, publishedVersion: false };
- }
+ let latest: string;
+ try {
+ latest = execFileSync(
+ "npm",
+ ["view", packageName, "version", "--json"],
+ {
+ stdio: "pipe",
+ }
+ )
+ .toString()
+ .trim();
+ } catch (_e) {
+ return { exists: false, publishedVersion: false };
+ }
+
+ if (latest === "" || latest === "undefined") {
+ return { exists: false, publishedVersion: false };
+ }
+
+ let publishedVersion = false;
+ try {
+ const specific = execFileSync(
+ "npm",
+ ["view", `${packageName}@${version}`, "version", "--json"],
+ { stdio: "pipe" }
+ )
+ .toString()
+ .trim();
+ publishedVersion = specific !== "" && specific !== "undefined";
+ } catch (_e) {
+ publishedVersion = false;
+ }
+
+ return {
+ exists: true,
+ publishedVersion,
+ };
}🤖 Prompt for AI Agents
In `@scripts/check-published.ts` around lines 34 - 60, The current try/catch wraps
both the package existence check and the version-specific lookup so when
execFileSync for `npm view ${packageName}@${version}` throws it incorrectly sets
`exists` to false; modify the code to perform the existence check (calling
`execFileSync` for `latest` and inspecting `latest`) inside its own try/catch to
set `exists`, then separately run the version-specific `execFileSync` (assign to
`specific`) inside its own try/catch that only sets `publishedVersion` based on
whether the call succeeds and `specific` is non-empty; keep using the existing
identifiers `execFileSync`, `packageName`, `version`, `latest`, `specific`,
`exists`, and `publishedVersion` so a thrown error on the version lookup does
not flip `exists`.
- Use nullish checks for gzipSize so 0 displays as '0 B' not '-' - Wrap Octokit API calls in try/catch for graceful failure on forked PRs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2793 +/- ##
==========================================
- Coverage 99.54% 99.37% -0.17%
==========================================
Files 58 63 +5
Lines 1105 1287 +182
Branches 335 397 +62
==========================================
+ Hits 1100 1279 +179
- Misses 5 8 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
Merge develop branch into main for release.
New Features
allowEmptyOutputoption@node-minify/action)silenceoption for run utilityBug Fixes
.action()Performance
fs.mkdirRefactoring
__dirnametoimport.meta.urlin tests (refactor(tests): modernize __dirname to import.meta.url #2781)Dependencies
oxc-minifyoraterserbuntsdown@biomejs/biomeNotes
@node-minify/benchmarkhavingworkspace:*in published dependenciesSummary by cubic
Ships the new official GitHub Action (uses: srod/node-minify@v1), adds allowEmptyOutput to skip writing empty files, and introduces docs pagination. Also includes key fixes (CLI on Commander 14, GCC string flags, cssnano options, OXC sourcemaps), consistent error handling, and I/O/dependency updates.
New Features
Migration
Written for commit cc5fd76. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Deprecation
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.