Add loading time check on CI #2163
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces automated execution time tracking to the bundle-size CI workflow. It removes legacy size-limit entries, adds time-based threshold enforcement to prevent import execution time increases exceeding 20%, and restructures the build and comparison workflow to compute dynamic limits upfront and apply them during the build process. Changes
Sequence DiagramsequenceDiagram
participant GHA as GitHub Actions
participant Script as bin/compare-bundle-sizes
participant NodeScript as scripts/bundle-size.mjs
participant Build as build-for-size-limit
participant SizeLimit as size-limit tool
participant Report as Report Results
GHA->>Script: Trigger bundle-size workflow
Script->>NodeScript: Run set-limits (base branch data)
NodeScript->>NodeScript: Calculate size + time limits<br/>(apply 20% time threshold)
NodeScript->>GHA: Save limits to /tmp/.size-limit-with-limits.json
Script->>Build: Execute build-for-size-limit
Build->>Build: Copy limits from /tmp to .size-limit.json
Build->>SizeLimit: Run standard build with new limits
SizeLimit->>SizeLimit: Measure bundle size & execution time
SizeLimit->>SizeLimit: Compare against dynamic limits
SizeLimit->>Report: Pass/Fail result
Report->>GHA: Report to PR
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Code Review FeedbackSummaryThis PR deletes a temporary file ( Issues Identified1. Misleading PR Title
|
size-limit report 📦
|
Updated Code Review - Multi-Commit PRI see this PR actually contains two separate changes across two commits: Commit 1: "reverted temp.js"
Commit 2: "don't run code on non-time limited entries"This is the actual meaningful change and requires careful review: Changes in
|
Code ReviewSummaryThis PR adds running time checks to the bundle size CI workflow. The changes enable measurement of not just bundle size, but also loading/running time for JavaScript bundles. Positive Aspects
CRITICAL Issue: Workflow Logic BugProblem: The workflow has a config overwrite bug at line 114 in bundle-size.yml Current flow:
This is a BUG! The dynamic limits set in line 106 are being overwritten before the size check runs. Fix Required - Choose one: Option 1: Use regular build script build_script: buildOption 2: Update temp file after setting limits node scripts/bundle-size.mjs set-limits --base /tmp/base-sizes.json
cp .size-limit.json /tmp/size-limit-config.jsonOther IssuesMEDIUM: Path Assumptions
LOW: Missing Tests
LOW: Performance
Priority Actions
ConclusionThe core logic fix is correct, but there is a critical workflow bug where dynamic limits are overwritten. Please fix before merging. Great work on adding time measurement! |
Code Review FeedbackOverviewThis PR implements runtime performance checking in the bundle size CI workflow and includes cleanup of temporary files. Positives
Critical Issues1. Misleading PR Title
2. Missing PR Description Code Quality Issues3. Logic Bug Fix (scripts/bundle-size.mjs:104-105) The fix is correct - checking baseEntry.running instead of sizeLimitEntry.running. However, there's a design issue: we're always creating sizeLimitEntry with running: false, which means the output config won't have running: true even when time limits are added. Consider preserving the running flag from baseEntry. 4. Workflow Complexity The workaround of copying .size-limit.json to /tmp and back adds hidden coupling. If someone runs pnpm build-for-size-limit locally without the file in /tmp, it will fail silently. Recommendation: Add a safety check or document this dependency in comments. Best Practices5. Missing Tests 6. Commit Message Quality
Security and Performance
Action Items Before Merging
Overall: Solid bug fix with good workflow optimization. Main concerns are documentation and the subtle logic around the running flag. |
Code ReviewHi @AbanoubGhadban! I've reviewed PR #2163. Here's my feedback: 📋 SummaryThis PR fixes the bundle size CI workflow to properly check running time in addition to bundle size. The key fix is changing line 105 in ✅ Positive Findings
🔍 Potential Issues & Questions1. Missing Test CoverageThe // Test cases to consider:
// - baseEntry with running property
// - baseEntry without running property (the bug scenario)
// - Verify limit calculation is correct
// - Verify time threshold calculation2. File Path DependencyThe workflow now depends on
# Potential improvement:
- name: Set dynamic limits (base + 0.5KB)
run: node scripts/bundle-size.mjs set-limits --base /tmp/base-sizes.json
- name: Verify limits were set
run: test -f .size-limit.json || exit 1
- name: Copy .size-limit.json file with limits to tmp directory
run: cp .size-limit.json /tmp/.size-limit-with-limits.json3. Error Handling in build-for-size-limitThe new "build-for-size-limit": "cp /tmp/.size-limit-with-limits.json .size-limit.json && pnpm build"If 4. Documentation GapConsider adding a comment in 🎯 Testing RecommendationsBefore merging, verify:
🔐 Security & Performance✅ No security concerns 📝 Style & ConventionsAccording to CLAUDE.md:
💡 Suggestions for Follow-up
✨ Overall AssessmentThis is a good fix for a legitimate bug. The logic error would have prevented running time checks from working when base entries lacked running metrics. The workflow improvements are sensible. Recommendation: Approve with suggestions for follow-up improvements (tests, documentation). Great work tracking this down! 🎉 |
Code Review - PR #2163SummaryThis PR makes changes to the bundle size CI workflow and fixes a bug in the bundle size script logic. However, there are critical issues that need to be addressed before merging. 🚨 Critical Issues1. Major Workflow Bug - Runner Configurationruns-on: node20 # Line 39Problem:
Impact: This workflow will fail immediately with "Unable to resolve action" error. Fix Required: Change to: runs-on: ubuntu-22.04And add back the Node.js setup step that was removed: - name: Setup Node
uses: actions/setup-node@v4
with:
node-version: '22'2. Misleading PR Title
The running time check already exists (all entries in Recommended Title: "Fix bundle size workflow and running time check logic" 📝 Code Quality Issues3. Logic Fix is Correct but Needs Testing (scripts/bundle-size.mjs:105)// Changed from:
if (!sizeLimitEntry.running) {
// To:
if (!baseEntry.running) {✅ This fix is correct. It properly checks whether the base entry has running time data before attempting to create time-based limits. ❌ Missing tests: This critical logic has no unit tests. According to CLAUDE.md, infrastructure code should be tested. 4. Unnecessary Complexity in WorkflowThe workflow now:
Question: Why copy to /tmp if we're immediately using it in the next step? The Potential simplification: Just use regular 🐛 Additional Issues5. Missing Error Handling"build-for-size-limit": "cp /tmp/.size-limit-with-limits.json .size-limit.json && pnpm build"If run locally (outside CI), this will fail with "No such file or directory". Consider adding: "build-for-size-limit": "test -f /tmp/.size-limit-with-limits.json && cp /tmp/.size-limit-with-limits.json .size-limit.json && pnpm build"6. Empty PR DescriptionThe PR body is completely empty. According to CLAUDE.md best practices, please add:
✅ Positive Aspects
🧪 Testing RequiredBefore merging, please:
📋 Action ItemsMust fix before merge:
Should address: RecommendationBLOCK MERGE until critical issue #1 (invalid runner) is fixed. The workflow will not run with Once fixed and tested, this will be a good improvement. The core logic fix is sound. According to CLAUDE.md:
|
Code Review FeedbackThank you for working on adding running time checks to CI! Here's my detailed review: 🚨 Critical Issues1. Invalid GitHub Actions Syntax (Blocking)runs:
using: 'node20'This is incorrect syntax. The Fix: runs-on: ubuntu-22.04You accidentally replaced 2. Missing Node.js Setup StepYou removed the Node.js setup step but pnpm and other tools depend on it: # This was removed but is needed:
- name: Setup Node
uses: actions/setup-node@v4
with:
node-version: '22'Why this matters: While 🐛 Bugs & Logic Issues3. Incorrect Variable Reference in bundle-size.mjsLine 105 in if (!baseEntry.running) { // ✅ Correct - checking baseEntryGood catch! The original code had: if (!sizeLimitEntry.running) { // ❌ Wrong - sizeLimitEntry doesn't have 'running'This is a valid bug fix. However, it should be mentioned in the PR description.
|
Code Review: Add Running Time Check on CIOverviewThis PR disables running time limits in the CI bundle size checks while keeping the configuration in place for potential local use. The approach taken is pragmatic and addresses a real environmental difference between the limit-setting script and the size-limit action. ✅ Strengths
🔍 Issues & Suggestions1. Logic Issue in Time Limit Condition
|
AbanoubGhadban
changed the base branch from
add-running-time-check-on-ci-base
to
master
@size-limit/time is not accurate at all in detecting changes in runtime
|
Seems that |
Code Review - PR #2163: Add running time check on CI📊 SummaryThis PR extends the existing bundle size CI check to also monitor JavaScript execution time (loading + running). It tracks performance regressions by comparing the total execution time of client-side imports between the PR and base branch, failing if any import increases by more than 20%. ✅ Strengths
|
AbanoubGhadban
changed the base branch from
master
to
add-running-time-check-on-ci-base
Code Review: Add Running Time Check on CIOverviewThis PR adds execution time monitoring to the bundle size CI workflow, introducing a 20% threshold for performance regressions. The implementation is solid overall with good architectural decisions. ✅ Strengths
🔍 Issues & Suggestions1. Documentation Gap (Minor)The CONTRIBUTING.md section on "Bundle Size Checks" should be updated to mention execution time monitoring. Impact: Low - users might be confused when CI reports time-based failures without documentation. 2. Missing Error Handling for Time Limits (Medium)In Potential issue: If the base branch has Impact: Medium - edge case but could confuse contributors during workflow changes. 3. Temp File Cleanup (Low)The workflow creates Impact: Very Low - purely cosmetic since GitHub runners reset state. 4. Consider Logging Time Limit Value (Low)In Impact: Low - quality-of-life improvement for debugging CI failures. 🧪 Testing ConsiderationsQuestion: Has this been tested with an actual performance regression? The 20% threshold seems reasonable but might need tuning based on real-world variance. 🎯 Architectural Decisions
📝 Nitpicks
✅ VerdictApprove with minor suggestions. The core implementation is sound and the architectural decisions are well-reasoned. The identified issues are minor and do not block merging:
Recommendation: Merge as-is and address documentation in a follow-up, unless you want to add the error handling for completeness. |
Code Review - PR #2163I've reviewed this PR and have feedback on code quality, implementation approach, and potential issues. Positive Aspects
Critical Issues1. Workflow Complexity and Fragility (High Priority)The new workflow introduces a complex file copying pattern. Lines 108-109 copy config to /tmp, then line 117 uses a custom build script that copies it back. This creates circular file copying where the build script silently overwrites .size-limit.json. Problems:
Better approach: Check if size-limit-action supports a --config parameter to pass /tmp/.size-limit-with-limits.json directly, or inline the build steps instead of using the action. 2. Logic Error in bundle-size.mjs (Medium Priority)Line 105-106 checks baseEntry.running and baseEntry.loading to decide if timing should be enabled. This checks the base branch measurement instead of the current entry config (entry.running). Why it matters: If entry.running = true but baseEntry.running = false, timing limits won't be created, making behavior inconsistent with .size-limit.json config. Correct logic should check entry.running first, then verify baseEntry has timing data. 3. Defensive Defaults Have Side Effects (Low Priority)Line 109 silently defaults missing loading/running to 0. Previously this would error. This could hide configuration issues. Better: Explicitly check for undefined and warn. Minor Issues
Testing ConcernsPer CLAUDE.md: Build scripts MUST be tested manually. Required:
Questions: Has build-for-size-limit been tested locally? What happens if run outside CI? RecommendationRequest changes for issues 1 and 2. The core logic change (disabling running: false) is correct, but the CI workflow needs simplification to be maintainable. |
Code ReviewThank you for this PR! I've reviewed the changes and have the following feedback: ✅ Positive Aspects
🐛 Critical Issue - Duplicate Step in WorkflowLocation: There's a duplicate step that copies the size-limit config file: - name: Copy .size-limit.json file with limits to tmp directory
run: cp .size-limit.json /tmp/.size-limit-with-limits.json
- name: Copy .size-limit.json file with limits to tmp directory
run: cp .size-limit.json /tmp/.size-limit-with-limits.jsonAction Required: Remove one of these duplicate steps (lines 111-112).
|
Code Review Feedback for PR #2163Thanks for your work on improving the bundle size monitoring! I've reviewed the changes and have the following feedback: 🎯 Overall AssessmentThe PR effectively addresses the limitations of runtime execution time measurement in CI by focusing on download time monitoring instead. The changes are well-reasoned and properly documented. ✅ Strengths
🐛 Issues FoundCritical: Duplicate Step in Workflow# Lines 108-112 in .github/workflows/bundle-size.yml
- name: Copy .size-limit.json file with limits to tmp directory
run: cp .size-limit.json /tmp/.size-limit-with-limits.json
- name: Copy .size-limit.json file with limits to tmp directory
run: cp .size-limit.json /tmp/.size-limit-with-limits.jsonAction Required: Remove the duplicate step (lines 111-112). 🔍 Code Quality Observations1. Typo in Comment// scripts/bundle-size.mjs:21
// 10% is a big ratio, but the current approach is not accurate enough to detect rations less than thatSuggestion: Change "rations" to "ratios". 2. Logical Improvement in bundle-size.mjs// Line 105 - BEFORE
if (!baseEntry.running && !baseEntry.loading) {
return sizeLimitEntry;
}
// Line 109 - AFTER
const { loading = 0, running = 0 } = baseEntry;The logic change from Current behavior: Skip time limits if BOTH running AND loading are falsy Suggestion: Consider this more explicit check: if (!baseEntry.running && !baseEntry.loading) {
return sizeLimitEntry;
}
// Even better with the defaults:
const { loading = 0, running = 0 } = baseEntry;
if (loading === 0 && running === 0) {
return sizeLimitEntry;
}This makes the intent clearer: "only create time limit entries if we have actual time measurements." 3. Workflow Job RenameThe rename from 🧪 Testing RecommendationsBased on CLAUDE.md testing requirements:
📋 Pre-Commit ChecklistPer CLAUDE.md requirements:
🚀 Performance ConsiderationsThe change from 20% to 10% threshold ( 🔒 Security ConcernsNo security issues identified. The changes are limited to build/CI configuration and don't introduce any user-facing vulnerabilities. ✏️ Suggested Changes Summary
📝 Additional Notes
Overall, this is a solid PR that addresses a real CI reliability issue. Please fix the duplicate workflow step before merging. |
AbanoubGhadban
changed the title
AbanoubGhadban
merged commit 6bc3c0a
into
add-running-time-check-on-ci-base
Summary
@size-limit/timeplugin to measure the execution time of each client bundle and ensure it doesn't exceed the base branch execution time by more than 20%.@size-limit/timeplugin is not good at all at detecting that because of the runtime noise on CI.download timeof chunks at the browser for now, until finding another tool that run the code multiple time on CI and get average execution time to get rid of runtime noise.bin/compare-bundle-sizesscript, so if you can run it locally with a noise mitigated environment you can do that by changing therunningvalues at.size-limit.jsonfile totrue.comparecommand from thescripts/bundle-size.mjsscript as it's not needed and used the default comparison behavior at thesize-limitpackage.