change percventage to size in bytes#5385
Conversation
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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 |
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
There was a problem hiding this comment.
Actionable comments posted: 2
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/webpack/extract/__tests__/modules-packages-duplicate.ts (1)
198-205: Clarify ambiguous duplicate package count messaging.The text "Bundle contains 3 duplicate packages" (line 200) is ambiguous. Looking at the data:
package-c: 3 instances (2 extra copies)package-a: 2 instances (1 extra copy)This could mean:
- "3 duplicate instances" (2 + 1 extra copies)
- "3 duplicated packages" (counting distinct package names + instances)
The wording should clarify whether it refers to duplicate instances or distinct package names. Consider: "Bundle contains 2 packages with 3 duplicate instances" or "Bundle contains 3 total duplicate package instances (2 packages affected)".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/utils/src/report/__tests__/__snapshots__/create.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
README.md(1 hunks)packages/utils/jest-output.txt(1 hunks)packages/utils/src/webpack/extract/__tests__/modules-packages-duplicate.ts(3 hunks)packages/utils/src/webpack/extract/__tests__/modules.ts(2 hunks)packages/utils/src/webpack/extract/modules.ts(1 hunks)packages/utils/src/webpack/metrics.ts(1 hunks)
🔇 Additional comments (7)
packages/utils/src/webpack/metrics.ts (1)
65-71: LGTM! Metric configuration correctly updated for byte-based duplicate code.The changes appropriately reflect the transition from a percentage-based metric to a file-size metric:
- Description accurately describes the total size measurement
- Type change to
FileSizeensures proper formatting- Setting
biggerIsBetter: falsecorrectly treats higher duplicate values as regressionspackages/utils/src/webpack/extract/__tests__/modules.ts (2)
72-72: LGTM! Test expectation correctly updated to byte-based metric.The expected
duplicateCodevalue of 600 bytes is correct:
./module-b-c.css: 500 bytes × 1 duplicate instance = 500./module-b-c.js: 100 bytes × 1 duplicate instance = 100- Total: 600 bytes
162-162: LGTM! Test expectation correctly updated to byte-based metric.The expected
duplicateCodevalue of 5000 bytes is correct:
module-c: 5000 bytes appears in 2 chunks, resulting in 1 duplicate instance- Total: 5000 bytes
packages/utils/src/webpack/extract/modules.ts (2)
83-98: LGTM! Duplicate code calculation correctly updated to byte-based metric.The refactored logic correctly calculates the total size of duplicate modules:
- Line 83:
duplicateCodeSizeaccumulator replaces the previous percentage-based calculation- Line 97: Correctly sums
duplicateInstances * sizefor each duplicated module- For a module appearing in N chunks, it's duplicated (N-1) times, each contributing the full module size
This aligns with the updated test expectations and metric configuration.
112-112: LGTM! Metric value correctly returns raw byte size.The change from a computed percentage to the raw
duplicateCodeSizecorrectly implements the byte-based duplicate code metric as specified in the PR objectives.packages/utils/src/webpack/extract/__tests__/modules-packages-duplicate.ts (2)
7-7: LGTM! Removed exclusive test marker.Correctly removed
describe.only- exclusive test blocks should not be committed as they skip other tests in the suite.
31-37: Code formatting change.Reformatted function call to multi-line without changing logic.
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
Remove test artifact file from version control.
This file appears to be a test output artifact and should not be committed. Test output files should be excluded via .gitignore.
Apply this diff to remove the file:
-1
- Additionally, ensure your .gitignore includes patterns to exclude test output files:
+# Test outputs
+**/jest-output.txtCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/utils/jest-output.txt at lines 1-1, this is a test output artifact
that should be removed from version control; delete the file from the repo and
add appropriate ignore patterns to .gitignore (e.g., jest-output.txt,
**/test-output/**, *.output, or a test artifacts rule) so future test outputs
are not committed; ensure you remove the tracked file (git rm or remove and
commit) and commit the updated .gitignore so the artifact stays excluded.
|
@Joaco2603 Thank you for the PR! I will take a closer look at it in the coming days. |
#12695 Bundle Size — 382.66KiB (-0.02%).a702f49(current) vs 89d8cd8 master#12692(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
Bundle analysis report Branch Joaco2603:feature/duplicateCode_... Project dashboard Generated by RelativeCI Documentation Report issue |
Change summary
Summary by CodeRabbit