refactor: decompose complex functions in utils and benchmark#2780
refactor: decompose complex functions in utils and benchmark#2780
Conversation
|
|
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. 📝 WalkthroughWalkthroughExtracted and exported benchmark helpers (warmup, iterations, metric calculation, error/cleanup), refactoring runner.ts to use them; expanded compressor resolver into public try-resolve helpers (built-in, npm, local) and a sequenced resolve flow; added Brotli raw size helper and related tests and validations. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client
participant RES as resolveCompressor
participant BUILT as Built-in resolver
participant NPM as NPM resolver
participant FILE as Local-file resolver
CLI->>RES: resolve(name)
RES->>BUILT: tryResolveBuiltIn(name)
alt built-in found
BUILT-->>RES: CompressorResolution
RES-->>CLI: CompressorResolution
else not found
BUILT-->>RES: null
RES->>NPM: tryResolveNpmPackage(name)
alt npm found
NPM-->>RES: CompressorResolution
RES-->>CLI: CompressorResolution
else not found
NPM-->>RES: null
RES->>FILE: isLocalPath(name) ? tryResolveLocalFile(name)
alt local found
FILE-->>RES: CompressorResolution
RES-->>CLI: CompressorResolution
else
FILE-->>RES: null
RES-->>CLI: throw Error("could not resolve compressor")
end
end
end
sequenceDiagram
participant CLI as Client
participant RUN as Benchmark Runner
participant WU as runWarmup
participant IT as runIterations
participant MET as calculateCompressorMetrics
participant FS as Filesystem
CLI->>RUN: start benchmark(file, compressor)
RUN->>WU: runWarmup(compressor, file, count)
WU-->>RUN: warmup completed
RUN->>IT: runIterations(compressor, file, iterations)
IT->>FS: write temp outputs (per iteration)
IT-->>RUN: iteration times + final output path
RUN->>MET: calculateCompressorMetrics(times, output, options)
MET->>FS: compute gzip/brotli sizes (optional)
MET-->>RUN: metrics
RUN-->>CLI: report metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/**/__tests__/*.test.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/*/src/index.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (9)📓 Common learnings📚 Learning: 2026-01-09T07:41:29.716ZApplied to files:
📚 Learning: 2026-01-09T07:41:29.716ZApplied to files:
📚 Learning: 2026-01-09T07:41:29.716ZApplied to files:
📚 Learning: 2026-01-09T07:41:29.716ZApplied to files:
📚 Learning: 2026-01-09T07:41:29.716ZApplied to files:
📚 Learning: 2026-01-09T07:41:29.716ZApplied to files:
📚 Learning: 2026-01-09T07:41:29.716ZApplied to files:
📚 Learning: 2026-01-09T07:41:29.716ZApplied to files:
🧬 Code graph analysis (2)packages/utils/src/getFilesizeGzippedInBytes.ts (1)
packages/utils/__tests__/compressor-resolver.test.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (24)
✏️ Tip: You can disable this entire section by setting 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 |
Greptile SummaryThis PR refactors complex functions in
The refactoring improves code readability, testability, and maintainability while preserving existing behavior. All exported functions are now individually testable and reusable. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant resolveCompressor
participant tryResolveBuiltIn
participant tryResolveNpmPackage
participant tryResolveLocalFile
User->>resolveCompressor: resolve("terser")
resolveCompressor->>tryResolveBuiltIn: try built-in first
tryResolveBuiltIn-->>resolveCompressor: CompressorResolution | null
alt built-in found
resolveCompressor-->>User: return resolution
else built-in not found
resolveCompressor->>tryResolveNpmPackage: try npm package
tryResolveNpmPackage-->>resolveCompressor: CompressorResolution | null
alt npm package found
resolveCompressor-->>User: return resolution
else npm not found & isLocalPath
resolveCompressor->>tryResolveLocalFile: try local file
tryResolveLocalFile-->>resolveCompressor: CompressorResolution | throws
resolveCompressor-->>User: return resolution
else not found anywhere
resolveCompressor-->>User: throw Error
end
end
|
srod
force-pushed
the
refactor/p2-decompose-complex-functions
branch
from
1308d55 to
4d0921c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/benchmark/src/runner.ts`:
- Around line 140-147: The CompressorMetrics population is inconsistent: when
options.includeBrotli is true, set both metrics.brotliSize and
metrics.brotliBytes just like gzip does; call the proper raw-byte helper (e.g.,
add or use getFilesizeBrotliRaw to produce a numeric byte count and assign it to
metrics.brotliBytes) and keep getFilesizeBrotliInBytes (or an equivalent
human-readable formatter) to set metrics.brotliSize—i.e., mirror the gzip branch
(getFilesizeGzippedInBytes / getFilesizeGzippedRaw) for brotli by adding the
missing raw-byte helper or by using getFilesizeBrotliInBytes for bytes and
converting/formatting for brotliSize so both fields are populated.
🧹 Nitpick comments (1)
packages/utils/src/compressor-resolver.ts (1)
145-171: Consider consistency in handling invalid exports.When
extractCompressorreturnsnull(package loads but exports no valid compressor), this function returnsnullsilently, whereastryResolveNpmPackagethrows an explicit error. Since built-in packages are controlled by the monorepo, this is unlikely to occur in practice, but for defensive consistency you could throw here as well.This is a minor observation; the current behavior is acceptable since built-in packages should always have valid exports.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/benchmark/__tests__/runner-edge-cases.test.tspackages/benchmark/src/runner.tspackages/utils/__tests__/compressor-resolver.test.tspackages/utils/src/compressor-resolver.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/utils/tests/compressor-resolver.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use 4 spaces for indentation
Use double quotes for strings
Always include semicolons at end of statements
Use ES5 trailing commas in objects and arrays
Usenode:prefix for Node.js built-in imports (e.g.,node:child_process)
Use camelCase for function and variable names
Usetry/catchwithif (err instanceof Error)checks for error handling
UsePromise.allfor parallel operations where all must succeed, orPromise.allSettledwhen you need results regardless of individual failures
Limit concurrency to 5-10 for file operations and 2-4 for CPU-intensive compressions to avoid resource exhaustion
Use file operation utilities from@node-minify/utils:readFile,writeFile,getFilesizeInBytes,getContentFromFilesAsync,isValidFileAsync
Use deprecation warnings viawarnDeprecationfrom@node-minify/utilsfor deprecated packages
UseresolveCompressorandisBuiltInCompressorutilities from@node-minify/utilsfor dynamic compressor resolution
Files:
packages/benchmark/__tests__/runner-edge-cases.test.tspackages/benchmark/src/runner.tspackages/utils/src/compressor-resolver.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Usetypekeyword for type-only imports in TypeScript
Include.tsfile extension in local imports (e.g.,./setup.ts)
Use PascalCase for type and interface names
Files:
packages/benchmark/__tests__/runner-edge-cases.test.tspackages/benchmark/src/runner.tspackages/utils/src/compressor-resolver.ts
packages/**/__tests__/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
Write tests using Vitest with
describe/test/expectin files located atpackages/*/__tests__/*.test.ts
Files:
packages/benchmark/__tests__/runner-edge-cases.test.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: srod/node-minify PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T07:41:29.716Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `resolveCompressor` and `isBuiltInCompressor` utilities from `node-minify/utils` for dynamic compressor resolution
Learnt from: CR
Repo: srod/node-minify PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T07:41:29.716Z
Learning: The monorepo architecture has core packages (`core`, `utils`, `run`, `types`, `cli`, `benchmark`) with `core` depending on `utils` and `run`, and all compressor packages depending on `core`
Learnt from: CR
Repo: srod/node-minify PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T07:41:29.716Z
Learning: Applies to packages/{esbuild,google-closure-compiler,oxc,swc,terser,uglify-js,clean-css,cssnano,csso,lightningcss,html-minifier,jsonminify,no-compress}/src/index.ts : Compressor functions must export an async function matching the `Compressor` type that receives `{ settings, content }` and returns `{ code, map? }`
Learnt from: CR
Repo: srod/node-minify PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T07:41:29.716Z
Learning: Applies to packages/{esbuild,google-closure-compiler,oxc,swc,terser,uglify-js,clean-css,cssnano,csso,lightningcss,html-minifier,jsonminify,no-compress}/__tests__/*.test.ts : Use shared test fixtures from `tests/fixtures.ts` with `runOneTest` helper and test suites like `commonjs`, `commoncss`, `commonhtml`
📚 Learning: 2026-01-09T07:41:29.716Z
Learnt from: CR
Repo: srod/node-minify PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T07:41:29.716Z
Learning: Applies to packages/{esbuild,google-closure-compiler,oxc,swc,terser,uglify-js,clean-css,cssnano,csso,lightningcss,html-minifier,jsonminify,no-compress}/__tests__/*.test.ts : Use shared test fixtures from `tests/fixtures.ts` with `runOneTest` helper and test suites like `commonjs`, `commoncss`, `commonhtml`
Applied to files:
packages/benchmark/__tests__/runner-edge-cases.test.tspackages/benchmark/src/runner.ts
📚 Learning: 2026-01-09T07:41:29.716Z
Learnt from: CR
Repo: srod/node-minify PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T07:41:29.716Z
Learning: Applies to packages/{esbuild,google-closure-compiler,oxc,swc,terser,uglify-js,clean-css,cssnano,csso,lightningcss,html-minifier,jsonminify,no-compress}/src/index.ts : Compressor functions must export an async function matching the `Compressor` type that receives `{ settings, content }` and returns `{ code, map? }`
Applied to files:
packages/benchmark/__tests__/runner-edge-cases.test.tspackages/benchmark/src/runner.tspackages/utils/src/compressor-resolver.ts
📚 Learning: 2026-01-09T07:41:29.716Z
Learnt from: CR
Repo: srod/node-minify PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T07:41:29.716Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `resolveCompressor` and `isBuiltInCompressor` utilities from `node-minify/utils` for dynamic compressor resolution
Applied to files:
packages/benchmark/__tests__/runner-edge-cases.test.tspackages/benchmark/src/runner.tspackages/utils/src/compressor-resolver.ts
📚 Learning: 2026-01-09T07:41:29.716Z
Learnt from: CR
Repo: srod/node-minify PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T07:41:29.716Z
Learning: Applies to packages/**/__tests__/*.test.ts : Write tests using Vitest with `describe`/`test`/`expect` in files located at `packages/*/__tests__/*.test.ts`
Applied to files:
packages/benchmark/__tests__/runner-edge-cases.test.ts
📚 Learning: 2026-01-09T07:41:29.716Z
Learnt from: CR
Repo: srod/node-minify PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T07:41:29.716Z
Learning: The monorepo architecture has core packages (`core`, `utils`, `run`, `types`, `cli`, `benchmark`) with `core` depending on `utils` and `run`, and all compressor packages depending on `core`
Applied to files:
packages/benchmark/__tests__/runner-edge-cases.test.tspackages/benchmark/src/runner.tspackages/utils/src/compressor-resolver.ts
📚 Learning: 2026-01-09T07:41:29.716Z
Learnt from: CR
Repo: srod/node-minify PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T07:41:29.716Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use file operation utilities from `node-minify/utils`: `readFile`, `writeFile`, `getFilesizeInBytes`, `getContentFromFilesAsync`, `isValidFileAsync`
Applied to files:
packages/benchmark/__tests__/runner-edge-cases.test.tspackages/benchmark/src/runner.ts
🧬 Code graph analysis (2)
packages/benchmark/__tests__/runner-edge-cases.test.ts (1)
packages/benchmark/src/runner.ts (2)
runIterations(71-99)calculateCompressorMetrics(111-150)
packages/benchmark/src/runner.ts (6)
packages/types/src/types.d.ts (1)
Compressor(83-84)packages/benchmark/src/types.ts (2)
BenchmarkOptions(7-20)CompressorMetrics(22-37)packages/core/src/index.ts (1)
minify(21-27)packages/benchmark/src/metrics.ts (1)
calculateReduction(7-13)packages/utils/src/getFilesizeGzippedInBytes.ts (2)
getFilesizeGzippedInBytes(49-60)getFilesizeGzippedRaw(70-80)packages/utils/src/getFilesizeBrotliInBytes.ts (1)
getFilesizeBrotliInBytes(17-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: test (ubuntu-latest, 24.x)
- GitHub Check: test (windows-latest, 20.x)
- GitHub Check: test (windows-latest, 22.x)
- GitHub Check: test (ubuntu-latest, 20.x)
- GitHub Check: test (windows-latest, 24.x)
- GitHub Check: test (ubuntu-latest, 22.x)
🔇 Additional comments (13)
packages/utils/src/compressor-resolver.ts (5)
7-9: LGTM!Imports correctly use the
node:prefix for Node.js built-ins and thetypekeyword for type-only imports, following the coding guidelines.
66-73: LGTM!The export of
isLocalPathis appropriate for the public API, and the Windows path detection regex correctly handles both forward and back slashes.
180-209: LGTM!The error handling correctly distinguishes between "package not found" (returns
nullto allow fallback) and "package found but invalid exports" (throws to surface the issue). Theif (error instanceof Error)pattern follows the coding guidelines.
218-255: LGTM!The use of
pathToFileURLensures proper ESM import compatibility across platforms. The error handling correctly surfaces invalid-export errors while providing context for file-not-found scenarios.Minor note: The error message on lines 240-241 mentions only "default export or named export 'compressor'", but
extractCompressoralso tries camelCase exports and iterates function exports. Consider aligning the message with actual behavior if you want more precise diagnostics.
264-294: Clean orchestration with appropriate resolution order.The sequential resolution strategy (built-in → npm → local) is logical and efficient. The defensive null check on lines 282-284 with its explanatory comment is a reasonable safeguard, even though
tryResolveLocalFilewill throw rather than returnnullfor actual local paths.packages/benchmark/src/runner.ts (6)
35-51: LGTM!The warmup helper is well-structured with proper type picking and clear documentation. The loop correctly stabilizes JIT and caches before timed iterations.
56-99: LGTM!Good input validation for
iterationCountand clean separation of timing logic. Usingperformance.now()is appropriate for high-resolution benchmarking.
159-172: LGTM!Clean error metrics factory with proper handling of both string and Error types.
179-187: LGTM!Appropriate error suppression for cleanup operations where partial failures are acceptable.
276-330: LGTM!The refactored
benchmarkCompressoris much cleaner with clear orchestration of the extracted helpers. Good patterns: unique temp file naming, cleanup in both success and error paths, and consistent error handling viacreateErrorMetrics.
338-374: LGTM!Good handling of the empty results edge case and clean use of
reducefor finding best metrics.packages/benchmark/__tests__/runner-edge-cases.test.ts (2)
11-11: LGTM!Correct import of the newly exported helper functions with
.tsextension per coding guidelines.
281-305: LGTM!Good validation test coverage using
rejects.toThrowwith exact error message matching. The mock compressor approach is appropriate since the validation occurs before the compressor is invoked.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 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/utils/src/getFilesizeBrotliInBytes.ts">
<violation number="1" location="packages/utils/src/getFilesizeBrotliInBytes.ts:27">
P2: Same error double-wrapping issue as `getFilesizeBrotliInBytes`: `getBrotliSize` throws `FileOperationError` for file validation failures, but this catch block unconditionally wraps all errors. Add a check to re-throw `FileOperationError` instances directly.</violation>
<violation number="2" location="packages/utils/src/getFilesizeBrotliInBytes.ts:62">
P2: Error double-wrapping regression: `getBrotliSize` already throws `FileOperationError` for missing/invalid files, but this catch block wraps it in another `FileOperationError`. The original code explicitly preserved `FileOperationError` instances with `if (error instanceof FileOperationError) { throw error; }`. Consider restoring that check to avoid nested error objects.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@greptile |
- Extract tryResolveBuiltIn, tryResolveNpmPackage, tryResolveLocalFile from resolveCompressor - Export isLocalPath utility for path detection - Extract runWarmup, runIterations, calculateCompressorMetrics from benchmarkCompressor - Add createErrorMetrics and cleanupTempFiles helpers - Add 15 new tests for extracted resolution functions resolveCompressor now delegates to focused single-responsibility functions. benchmarkCompressor now ~30 lines vs ~97 lines before.
srod
force-pushed
the
refactor/p2-decompose-complex-functions
branch
from
ceaeece to
1716594
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2780 +/- ##
===========================================
- Coverage 99.59% 99.37% -0.22%
===========================================
Files 63 63
Lines 1242 1286 +44
Branches 380 390 +10
===========================================
+ Hits 1237 1278 +41
- Misses 5 8 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
resolveCompressorinto 3 focused functionsbenchmarkCompressorinto 5 focused functionsChanges
@node-minify/utils- compressor-resolver.tstryResolveBuiltIn(name)tryResolveNpmPackage(name)tryResolveLocalFile(name)isLocalPath(name)resolveCompressornow orchestrates these three in sequence (~15 lines vs ~95 before).@node-minify/benchmark- runner.tsrunWarmup(...)runIterations(...)calculateCompressorMetrics(...)createErrorMetrics(name, error)cleanupTempFiles(files)benchmarkCompressornow ~30 lines vs ~97 before.Testing
Implements P2 from REFACTORING_PLAN.md
Summary by cubic
Split compressor resolution and benchmarking into small, focused functions to improve readability and testability. Adds input validation, brotli raw size support, and targeted tests.
Written for commit 1716594. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.