fix: handle empty content and output routing regressions#2810
Merged
Conversation
🦋 Changeset detectedLatest commit: badb0db 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 |
Review Summary by QodoFix empty content handling, output routing, and subprocess event handling
WalkthroughsDescription• Handle empty string content as valid in-memory input mode • Fix multi-format output routing for indexed array inputs • Wait for subprocess close event before reading final output • Use benchmark defaults when compressors not specified Diagramflowchart LR
A["Empty String Content"] -->|"Check !== undefined"| B["Valid In-Memory Mode"]
C["Multi-Format Outputs"] -->|"Use Current Index"| D["Route to Correct Input"]
E["Subprocess Exit"] -->|"Wait for Close Event"| F["Read Final Output"]
G["Benchmark CLI"] -->|"No Compressors Arg"| H["Apply Defaults"]
File Changes1. packages/core/src/index.ts
|
Code Review by Qodo
1.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors input/output resolution and in-memory content checks (empty string now treated as provided), switches child-process finalization from Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (benchmark)
participant Program as Runner/Utils
participant Child as ChildProcess
participant FS as FileSystem
CLI->>Program: run benchmark with options (compressors, type, format)
Program->>Child: spawn worker process
Child-->>Program: stdout chunks
Child-->>Program: close event
Program->>Program: finalize buffers, resolve result on close
Program->>FS: writeMultipleOutputs (resolve targets, expand "$1", append extensions)
FS-->>Program: write complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+25
to
+28
| const method = | ||
| compressorSettings.content !== undefined | ||
| ? compressSingleFile | ||
| : compress; |
There was a problem hiding this comment.
2. No changeset added 📘 Rule violation ⛯ Reliability
This PR includes user-facing behavior changes but does not add a changeset file for release notes/versioning. This violates the requirement to include a changeset for user-facing changes.
Agent Prompt
## Issue description
User-facing behavior changes were made without adding a changeset file.
## Issue Context
Changeset files are required for release notes/versioning whenever user-facing behavior changes.
## Fix Focus Areas
- packages/core/src/index.ts[25-28]
- packages/cli/src/bin/cli.ts[96-112]
- packages/utils/src/run.ts[176-233]
- packages/run/src/index.ts[99-99]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Greptile SummaryThis PR fixes four independent regressions across the monorepo with targeted test coverage.
All four fixes are well-scoped, directly address documented regressions, and are covered by new tests. Confidence Score: 5/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["minify(settings)"] --> B["setup(settings)"]
B --> C{settings.content\n!== undefined?}
C -- Yes --> D["validateMandatoryFields\n(['compressor'])"]
D --> E["compressSingleFile(settings)"]
C -- No --> F["validateMandatoryFields\n(['compressor','input','output'])"]
F --> G["enhanceSettings(settings)"]
G --> H["compress(settings)"]
E --> I["determineContent(settings)"]
I --> J{content\n!== undefined?}
J -- Yes, even empty string --> K["return settings.content"]
J -- No --> L["read from input file(s)"]
K --> M["run({ settings, content })"]
L --> M
M --> N["compressor(...)"]
N --> O["writeOutput(result, settings, index)"]
O --> P{settings.content\n!== undefined?}
P -- Yes, in-memory → skip write --> Q["return result.code"]
P -- No --> R{result.outputs?}
R -- Yes --> S["writeMultipleOutputs(outputs, settings, index)"]
R -- No --> T["writeFileAsync(output, code)"]
S --> U{index defined?}
U -- Yes --> V["currentOutput = output[index]\nauto-append .format extension"]
U -- No, perFormatOutputs --> W["arrayItem = output[i]\nuse explicit path per format"]
V --> Q
W --> Q
T --> Q
subgraph "run/src/index.ts — Java subprocess"
X["spawn java"] --> Y["listen to stdout/stderr data"]
Y --> Z["child.on('close', ...)"]
Z --> AA["resolve(stdout)"]
end
Last reviewed commit: a50e294 |
Treat empty string content as valid in-memory input, resolve multi-format outputs against the current input entry, make benchmark CLI use benchmark defaults, and wait for java processes to close before reading final output.
Suppress spinner output for JSON benchmark runs, add the required changeset, remove the new type suppression in utils tests, and update the indexed input helper docs.
srod
force-pushed
the
codex/fix-review-findings
branch
from
82b7fc6 to
43c11f7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/utils/src/run.ts`:
- Around line 202-228: The per-format output branch (using perFormatOutputs /
arrayItem / output[i]) treats entries like "dist/$1.webp" as literal paths and
skips $1 substitution; update the logic in the initial branch that sets
targetFile from arrayItem so that if arrayItem is a string containing "$1" you
replace all "$1" with the computed inputBase (falling back to "output") before
deciding targetFile, and if the resulting value does not already include the
format extension append `.${format}` when appropriate (mirror the behavior in
the currentOutput includes("$1") branch), using the same helpers (inputBase,
inputDir, format, join) and preserving the existing checks for
undefined/trim/"$1".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ce67335-e139-434a-953d-f74ac250d5d8
⛔ Files ignored due to path filters (1)
.changeset/fresh-tomatoes-crash.mdis excluded by!.changeset/**
📒 Files selected for processing (10)
packages/cli/__tests__/cli-benchmark-bin.test.tspackages/cli/src/bin/cli.tspackages/core/__tests__/core.test.tspackages/core/src/index.tspackages/core/src/setup.tspackages/run/__tests__/run.test.tspackages/run/src/index.tspackages/utils/__tests__/utils.test.tspackages/utils/src/compressSingleFile.tspackages/utils/src/run.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/run/src/index.ts
- packages/cli/tests/cli-benchmark-bin.test.ts
- packages/core/src/index.ts
- packages/core/tests/core.test.ts
- packages/utils/tests/utils.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/utils/src/run.ts`:
- Around line 229-235: The code handling indexed `$1` output patterns (in the
branch using currentOutput.includes("$1") in run.ts) blindly appends
`.${format}` after replacing `$1`, causing doubled extensions like
`dist/foo.webp.webp`; update the logic that sets targetFile (after const
outputFile = currentOutput.replace(/\$1/g, baseName)) to only append
`.${format}` when outputFile does not already end with `.${format}` (use a
case-sensitive endsWith check), and ensure this mirrors the extension-guard
behavior used in the per-format-array branch; also add a regression test for an
indexed pattern like "dist/$1.webp" with index set to confirm no
double-extension occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3ad6992-168f-4604-8820-c792de50cec5
📒 Files selected for processing (2)
packages/utils/__tests__/utils.test.tspackages/utils/src/run.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/utils/src/run.ts`:
- Around line 236-240: The branch handling a single string output currently
appends `.${format}` directly causing duplicated extensions for explicit
filenames like "dist/foo.webp"; update the branch that sets `targetFile` (using
the variables `currentOutput`, `format`, and `targetFile`) to call the existing
helper `withFormatExtension(currentOutput, format)` instead of
string-concatenating `.${format}` so explicit extensions are preserved; ensure
you reuse `withFormatExtension()` logic exactly as used elsewhere in this module
to normalize both plain-string and indexed multi-format outputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9804469c-5b2e-45bc-a0a8-4f3e1accf5dc
📒 Files selected for processing (2)
packages/utils/__tests__/utils.test.tspackages/utils/src/run.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/utils/src/run.ts`:
- Around line 207-219: The per-format array branch currently uses explicit array
entries (arrayItem) as-is and therefore skips appending the format extension;
update the branch inside the perFormatOutputs handling so that after computing
targetFile (both in the arrayItem.includes("$1") and the "else" explicit path)
you pass the resolved path through withFormatExtension(targetFile, format) (or
call withFormatExtension on arrayItem before assigning to targetFile) ensuring
entries like "dist/foo" become "dist/foo.webp" or "dist/foo.avif"; also add a
regression test for an array-style per-format target with no extension (e.g.,
["dist/foo"]) verifying the correct format extension is appended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3d8e9dc-b783-4ff5-9f5a-f2e1d39d843c
📒 Files selected for processing (2)
packages/utils/__tests__/utils.test.tspackages/utils/src/run.ts
Comment on lines
207
to
+219
| if ( | ||
| isArrayOutput && | ||
| perFormatOutputs && | ||
| arrayItem !== undefined && | ||
| arrayItem !== "$1" && | ||
| arrayItem.trim() !== "" | ||
| ) { | ||
| // Explicit output path provided in array - use as-is | ||
| targetFile = arrayItem; | ||
| } else if (typeof output === "string" && output === "$1") { | ||
| if (arrayItem.includes("$1")) { | ||
| const outputFile = arrayItem.replace(/\$1/g, baseName); | ||
| targetFile = withFormatExtension(outputFile, format); | ||
| } else { | ||
| // Explicit output path provided in array - use as-is | ||
| targetFile = arrayItem; | ||
| } |
Contributor
There was a problem hiding this comment.
Append the format for explicit per-format array targets too.
In this branch, entries like ["dist/foo", "dist/bar"] still bypass withFormatExtension(), so they write to dist/foo/dist/bar instead of dist/foo.webp/dist/bar.avif. That leaves per-format output arrays inconsistent with the single-string and indexed branches, and it misses the PR’s auto-extension behavior.
Suggested fix
if (
perFormatOutputs &&
- arrayItem !== undefined &&
+ typeof arrayItem === "string" &&
arrayItem !== "$1" &&
arrayItem.trim() !== ""
) {
if (arrayItem.includes("$1")) {
const outputFile = arrayItem.replace(/\$1/g, baseName);
targetFile = withFormatExtension(outputFile, format);
} else {
- // Explicit output path provided in array - use as-is
- targetFile = arrayItem;
+ targetFile = withFormatExtension(arrayItem, format);
}
} else if (Please add a regression test for the no-extension per-format-array case alongside this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/utils/src/run.ts` around lines 207 - 219, The per-format array
branch currently uses explicit array entries (arrayItem) as-is and therefore
skips appending the format extension; update the branch inside the
perFormatOutputs handling so that after computing targetFile (both in the
arrayItem.includes("$1") and the "else" explicit path) you pass the resolved
path through withFormatExtension(targetFile, format) (or call
withFormatExtension on arrayItem before assigning to targetFile) ensuring
entries like "dist/foo" become "dist/foo.webp" or "dist/foo.avif"; also add a
regression test for an array-style per-format target with no extension (e.g.,
["dist/foo"]) verifying the correct format extension is appended.
Owner
Author
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2810 +/- ##
===========================================
- Coverage 95.22% 95.21% -0.01%
===========================================
Files 73 73
Lines 1739 1757 +18
Branches 527 543 +16
===========================================
+ Hits 1656 1673 +17
- Misses 83 84 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--compressorsis passedclosebefore reading final stdout/stderrTesting
Summary by cubic
Fixes regressions across
@node-minify/core,@node-minify/utils,@node-minify/run, and@node-minify/cli. Accept empty in-memory content, route multi-format outputs per input with$1, preserve explicit extensions, and append missing format extensions for array targets; benchmark JSON is clean and subprocesses resolve onclose.@node-minify/core: Treat""as validcontent; use in-memory mode whencontent !== undefined.@node-minify/utils: Expand$1in per-format arrays and indexed runs; route outputs by current input; append missing extensions for array targets and$1patterns; avoid double-append for indexed$1; preserve explicit extensions for single-string outputs.@node-minify/run: Resolve oncloseto capture late stdout/stderr.@node-minify/cli: Benchmark uses defaults unless--compressorsis set; read--typefrom global options; suppress spinner for JSON and non‑TTY.Written for commit badb0db. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests