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 @coderabbitai help to get the list of available commands and usage tips.}

Code Review: Add Running Time Check on CI

Summary

This PR adds execution time tracking to the bundle size CI checks by integrating @size-limit/time plugin and modifying the comparison workflow to track both bundle size and execution time (loading + running).

Critical Issues

1. Incomplete PR Description

The PR body still contains template placeholder text. Please update with:

• What problem this solves
• Why tracking running time is needed
• What the expected behavior change is

2. Missing Test Coverage

According to the PR checklist, these items are unchecked:

• Add/update test to cover these changes
• Update documentation
• Update CHANGELOG file

Questions:

• How can we verify the time tracking works correctly?
• Should there be unit tests for the createLimitEntry function?

3. Missing CHANGELOG Entry

Per CLAUDE.md requirements, user-visible changes need CHANGELOG entries. This is a developer-visible change that should be documented in /CHANGELOG.md.

4. Inconsistent running Flag Configuration

In .size-limit.json, only ONE entry has running: true (line 122: wrapServerComponentRenderer/client bundled brotli). All other 16 entries have running: false.

Questions:

• Is this intentional? Why only track runtime for this one specific bundle?
• Should all bundled entries have running: true?

Code Quality Issues

5. Magic Number for Time Threshold

DEFAULT_TIME_PERCENTAGE_THRESHOLD = 0.2 is hardcoded without explanation. Consider adding a comment explaining why 20% is appropriate.

6. Missing Error Handling for Time Data

In createLimitEntry at line 107, if baseEntry does not have loading or running fields, this will produce NaN and cause silent failures. Add validation to check for these fields before using them.

7. Function Returns Inconsistent Types

createLimitEntry returns either an object or an array of 2 objects. This is confusing. Consider always returning an array for consistency.

Additional Observations

8. Dependency Addition Impact

@size-limit/time adds Puppeteer and 532 lines to pnpm-lock.yaml. Please verify:

• CI build time impact (Puppeteer needs Chrome installation)
• Whether this increases CI costs

9. Documentation Gaps

The CONTRIBUTING.md update mentions total execution time but lacks:

• Explanation of what these metrics mean
• Guidance on acceptable thresholds
• Troubleshooting steps

Positive Aspects

1. Good separation of concerns - bundle-size.mjs is well-structured
2. Helpful output with color-coded console messages
3. Backward compatible - entries without running: true continue to work
4. Simplifies workflow by removing custom comparison logic

Recommendations

Must Fix Before Merge:

1. Update PR description with actual content
2. Add CHANGELOG entry
3. Add error handling for missing time data
4. Explain running: true configuration choice
5. Complete all PR checklist items

Should Consider:
6. Document time threshold rationale
7. Add unit tests for createLimitEntry
8. Update CONTRIBUTING.md with detailed time tracking guidance
9. Verify CI build time impact

Final Verdict

Status: Needs Work

The core implementation looks solid and this is a useful feature. However, the PR needs:

• Complete documentation
• Error handling for edge cases
• Clarification on configuration choices

Once these are addressed, this will be ready to merge.

Code Review

Thank you for this PR! I've reviewed the changes to add runtime performance checks to the bundle size CI workflow. Here's my feedback:

✅ Strengths

1. Good architectural decision: Simplifying the workflow by removing the separate compare command and leveraging size-limit's built-in checking is cleaner and more maintainable.

2. Valuable addition: Tracking runtime performance (loading + running time) alongside bundle size is important for catching performance regressions.

3. Smart threshold approach: Using a percentage-based threshold (20%) for time is appropriate given that timing can be more variable than file size.

4. Cleanup handling: Good addition of git restore .size-limit.json in the cleanup function to ensure the config is restored after comparisons.

⚠️ Issues to Address

1. PR Description Missing (CRITICAL)

The PR description template hasn't been filled out. Please provide:

• Summary: Explain what this PR does and why (e.g., "Adds runtime performance checks to bundle size CI to catch performance regressions early")
• Motivation: Why is checking runtime important?
• Checklist: Complete the checklist items:
  • Tests: Are there tests for the script changes?
  • Documentation: CONTRIBUTING.md was updated ✅
  • CHANGELOG: Missing! This is a user-visible change that should be documented

2. CHANGELOG Entry Required

Per CLAUDE.md:

IMPORTANT: Update CHANGELOG.md for user-visible changes only (features, bug fixes, breaking changes, deprecations, performance improvements)

This is a user-visible change (the bin/compare-bundle-sizes script now also checks runtime). Add an entry under Developer Experience section.

🔍 Code Quality Issues

a) Magic numbers without explanation (scripts/bundle-size.mjs:108-109)

const loadingMs = loading * 1000;
const runningMs = running * 1000;

The code assumes loading and running are in seconds, but this isn't documented. Add a comment explaining the units.

b) Potential undefined property access (line 107)

const { loading, running } = baseEntry;

If baseEntry doesn't have loading or running properties (e.g., base was measured without runtime), this will silently create NaN values. Add validation:

if (!baseEntry.loading || !baseEntry.running) {
  console.warn('Warning: No runtime data, skipping time limit');
  return sizeLimitEntry;
}

c) Help text could be clearer (line 178)

--timePercentageThreshold <ratio between 0 and 1>

Should specify the default:

--timePercentageThreshold <number>  Time increase threshold as decimal (0.2 = 20%, default: 0.2)

🧪 Testing Required

Per CLAUDE.md - manual testing is required before merge:

# Test the full workflow
bin/compare-bundle-sizes

# Test with a branch that has different bundle sizes
git checkout -b test-bundle-changes
# Make a small change to trigger size difference
bin/compare-bundle-sizes master

Please confirm you've tested locally and share the results.

🎯 Recommendations (Optional)

1. Add validation for percentage threshold:

   if (timePercentageThreshold < 0 || timePercentageThreshold > 1) {
     console.error('Error: timePercentageThreshold must be between 0 and 1');
     process.exit(1);
   }

2. Consider using flatMap instead of flat (line 155) for clarity:

   .flatMap((entry) => {
     const result = createLimitEntry(entry, baseEntry, threshold, timePercentageThreshold);
     return Array.isArray(result) ? result : [result];
   })

📋 Action Items Before Merge

• Fill out PR description completely
• Add CHANGELOG entry
• Add validation for missing runtime data in createLimitEntry
• Test locally with bin/compare-bundle-sizes and share results
• Add comment explaining units (seconds → milliseconds conversion)
• Improve help text to show default value

Great work on improving the CI tooling! 🚀

Code Review - PR #2160: Add Running Time Check on CI

This PR adds execution time monitoring to bundle size CI checks. Good simplification by removing the compare command (~110 lines), but needs proper documentation before merge.

✅ Strengths

1. Simplifies code by leveraging size-limit's built-in reporting
2. Reasonable 20% time threshold for performance regression detection
3. Selective monitoring (only Pro packages with running: true)
4. Properly restores .size-limit.json in cleanup

⚠️ Critical Issues

1. Missing PR Description - Still using template placeholders. Must explain:

• What problem this solves
• Why 20% threshold was chosen
• Which packages are monitored

2. Missing CHANGELOG Entry - No entry for this PR despite update changelog commit. Add under Unreleased > Improved section.

3. Missing Test Evidence - Per CLAUDE.md, infrastructure code MUST be tested manually. Document results of:

• Clean install (rm -rf node_modules && pnpm install --frozen-lockfile)
• Build success
• Bundle size script generates correct time limits
• size-limit accepts new format

Other Recommendations

• Consider consistent return type in createLimitEntry (always return array)
• Add comment explaining time entry format compatibility with size-limit
• Make git restore more robust: git restore .size-limit.json 2>/dev/null || true
• Clarify help text for --timePercentageThreshold option

Action Items Before Merge

• CRITICAL: Fill out PR description
• CRITICAL: Add CHANGELOG entry
• REQUIRED: Document local testing results
• Consider code improvements above

Recommendation: Request changes to complete documentation before merging.

PR Review: Add running time check on CI

Thank you for adding execution time monitoring! This is a valuable addition to catch performance regressions early. Here's my detailed feedback:

Overall Assessment

Strengths:

• Smart approach extending the existing bundle-size infrastructure
• Good simplification by removing the compare command and letting size-limit handle validation
• Well-chosen 20% threshold for initial rollout (acknowledging measurement accuracy limitations)

Critical Issues:

1. Incomplete PR checklist - needs addressing before merge
2. Potential breaking change in bin/compare-bundle-sizes - needs testing
3. Missing error handling for base entries without timing data
4. Inconsistent running configuration needs explanation

Critical Issues

1. PR Checklist Not Completed

The PR body still contains placeholder text with unchecked items.

Per CLAUDE.md requirements, you must:

• Remove placeholder text from PR description
• Complete or strike through (~) inapplicable items
• Document what was tested and how

2. Missing Local Testing Documentation

CRITICAL per CLAUDE.md Section: LOCAL TESTING REQUIREMENTS

The PR doesn't document:

• Whether you tested bin/compare-bundle-sizes locally
• Whether size-limit properly fails when time limits are exceeded
• Whether the 20% threshold works as expected
• Test results showing before/after behavior

3. Breaking Change in bin/compare-bundle-sizes

Lines 28, 66-70: The script now modifies .size-limit.json during execution and restores it in cleanup. This is a behavior change that:

• Could leave the file in a modified state if the script crashes before cleanup
• Wasn't present in the original implementation
• Needs explicit testing and documentation

Required: Test this script locally with an actual PR branch and document the temporary modification behavior.

High Priority Issues

4. Missing Error Handling for Base Entries

scripts/bundle-size.mjs:109-114:

If base branch measurement doesn't include loading/running (e.g., old commit, failed measurement), this will crash with TypeError: Cannot read properties of undefined.

Fix needed:

const { loading, running } = baseEntry;
if (loading === undefined || running === undefined) {
  console.log(`  ${colors.yellow}No timing data, skipping time limit${colors.reset}`);
  return sizeLimitEntry;
}

5. Inconsistent running Configuration

.size-limit.json shows:

• Lines 1-68: "running": false for raw/gzip/brotli measurements
• Lines 81-122: "running": true for only Pro package client imports

The CHANGELOG says:

Compares PR client import execution times

But only 6 out of 17 entries have running: true. This seems inconsistent.

Questions:

1. Why is running: true only for Pro packages, not open-source?
2. Is this intentional or an oversight?
3. Should react-on-rails/client bundled entries (lines 69-78) also have running: true?

Medium Priority Issues

6. Typo in Comment

scripts/bundle-size.mjs:21: "ration" should be "ratio"

7. CHANGELOG Formatting

The entry for PR 2160 is placed ABOVE PR 2149, but 2160 is newer. Consider clarifying the ordering strategy (chronological vs grouped by type).

8. Missing Test Coverage

Per CLAUDE.md: No tests were added for:

• createLimitEntry() function with running: true
• Time percentage threshold calculation
• .flat() behavior when returning arrays

Code Quality Suggestions

9. Simplification Opportunity

Consider extracting time entry creation to a helper function for clarity.

10. Documentation Enhancement

CONTRIBUTING.md:337: Add a note about which packages have timing checks:

Compares the size and total execution time (loading + running) for client-side imports with running: true in .size-limit.json

What's Done Well

1. Smart architectural decision: Leveraging size-limit's built-in running support
2. Good conservative threshold: 20% with acknowledgment of accuracy limitations
3. Clean code removal: Properly removed the no-longer-needed compare command
4. Appropriate use of .flat(): Handles conditional array returns elegantly
5. Consistent naming: (time) suffix clearly distinguishes timing entries

Pre-Merge Checklist

Before this can be merged, please:

• Complete the PR description (remove placeholder text)
• Document local testing performed
• Test bin/compare-bundle-sizes with the new behavior
• Add error handling for missing loading/running in base entries
• Clarify running: true configuration strategy
• Fix typo: "ration" → "ratio"
• Consider adding unit tests for the new functionality
• Verify CI passes with the new timing checks
• Run bundle exec rubocop and fix any violations (per CLAUDE.md)

Testing Commands You Should Run

Per CLAUDE.md requirements:

# 1. Test the compare script works
bin/compare-bundle-sizes

# 2. Test size-limit with new config
pnpm exec size-limit

# 3. Test that failures work (temporarily modify a limit)
# Edit .size-limit.json: set a time limit to 1ms, verify it fails

# 4. Run linting
bundle exec rubocop
pnpm run lint

# 5. Verify file ends with newline
tail -c 1 scripts/bundle-size.mjs | od -An -tx1

Let me know if you have questions about any of this feedback! Happy to discuss the running: true strategy or help with error handling implementation.

Code Review: Add Running Time Check on CI

✅ Overall Assessment

This is a well-structured PR that extends the existing bundle size monitoring to include execution time tracking. The implementation is solid, but there are several areas that need attention before merging.

🔴 Critical Issues

1. Incomplete PR Description

The PR body still contains template placeholders. This needs to be filled out with:

• Clear description of what this PR does
• Why execution time monitoring is needed (the "why" behind the change)
• How it works (brief technical overview)
• Examples of what developers will see

2. Missing Test Coverage

The PR checklist shows:

• Add/update test to cover these changes

Issue: There are no tests for the new createLimitEntry function or the time limit calculation logic. This is infrastructure code that affects CI, so it MUST be tested.

Recommendation: Add unit tests for:

• createLimitEntry with and without running: true
• Time limit calculation with different thresholds
• Edge cases (zero time, very large times, etc.)

3. git restore May Fail Silently

Location: bin/compare-bundle-sizes:28

cleanup() {
  git restore .size-limit.json  # ← This could fail if file was not modified
  echo -e "\n${BLUE}Cleaning up...${NC}"

Issue: If .size-limit.json was not modified, git restore will fail and stop the cleanup function due to set -e.

Fix:

cleanup() {
  git restore .size-limit.json 2>/dev/null || true
  echo -e "\n${BLUE}Cleaning up...${NC}"

⚠️ Important Issues

4. 20% Threshold is Very High

Location: scripts/bundle-size.mjs:21-22

// 20% is a big ration, but the current approach is not accurate enough...
const DEFAULT_TIME_PERCENTAGE_THRESHOLD = 0.2;

Concerns:

• 20% is indeed very high for performance regression detection
• The comment admits "not accurate enough" - this suggests the approach may need refinement
• A 20% slowdown is significant and would likely be noticed by users before CI catches it

Questions:

1. What causes the inaccuracy? Is it variance in CI runners? Measurement methodology?
2. Have you measured the variance in execution time across multiple CI runs on the same commit?
3. Would running multiple iterations and averaging help?

Recommendation:

• Add documentation explaining the variance you have observed
• Consider adding a flag to make this configurable per-package
• Track this limitation for future improvement

5. Simplified Workflow May Hide Information

Location: bin/compare-bundle-sizes:66-70

The old workflow showed a comparison table. The new workflow just runs size-limit which will pass/fail but may not show the detailed comparison.

Before: Developers saw a detailed table of size differences
After: Developers see only pass/fail from size-limit

Question: Does pnpm exec size-limit show enough detail for developers to understand what changed?

Recommendation: Test this locally and ensure developers get actionable feedback.

📋 Code Quality Issues

6. Inconsistent Naming: running vs Execution Time

Location: .size-limit.json

The config uses "running": false but the feature tracks total execution time (loading + running).

Issue: The name running is ambiguous. Does it mean:

• Enable running time measurement?
• Track the "running" phase specifically?

Recommendation: Consider renaming to "trackExecutionTime": false or "measureTime": false for clarity.

7. Magic Number Without Clear Explanation

Location: scripts/bundle-size.mjs:99-101

const totalTime = loadingMs + runningMs;
return [
  sizeLimitEntry,
  {
    ...entry,
    name: `${entry.name} (time)`,
    limit: `${(totalTime * (1 + timePercentageThreshold)).toFixed(0)} ms`,
  },
];

Issue: This creates TWO entries (size + time) but it is not immediately obvious why. The duplication could be confusing when reading the config file output.

Recommendation: Add a comment explaining why we create duplicate entries:

// Create two entries: one for size limit, one for execution time limit
// size-limit processes these separately
return [sizeLimitEntry, timeEntry];

8. Removed compare Command Without Deprecation Notice

Location: scripts/bundle-size.mjs:4-12

The entire compare command was removed, but the commit/PR does not explain why.

Questions:

• Was this command still being used?
• Is the functionality now handled entirely by size-limit?
• Should we deprecate rather than remove?

Recommendation: Add a note in the PR description explaining this breaking change.

💭 Design Questions

9. Why Disable Time Tracking for All Entries?

All entries in .size-limit.json have "running": false.

Questions:

• Is this intentional for this PR (to add infrastructure without enabling it yet)?
• Which entries should have time tracking enabled?
• Should entries without import (raw file measurements) skip time tracking?

Observation: Only entries with "import" specified can measure execution time (you need to import and run code). Entries with just "path" measure file size only.

Current state:

• Lines 68-124: All have import → SHOULD enable time tracking
• Lines 2-67: No import → Cannot measure time (correctly disabled)

Recommendation: Consider enabling time tracking for entries that have "import":

{
  "name": "react-on-rails/client bundled (gzip)",
  "path": "packages/react-on-rails/lib/ReactOnRails.client.js",
  "import": "ReactOnRails",
  "gzip": true,
  "running": true
}

10. CI Workflow Not Updated

Location: .github/workflows/bundle-size.yml:105

The workflow says:

- name: Set dynamic limits (base + 0.5KB)
  run: node scripts/bundle-size.mjs set-limits --base /tmp/base-sizes.json

Issue: Comment is outdated - it is not just "base + 0.5KB" anymore, it also sets time limits.

Fix:

- name: Set dynamic limits (size + time)
  run: node scripts/bundle-size.mjs set-limits --base /tmp/base-sizes.json

✨ Positive Aspects

1. Good separation of concerns: Size and time limits are handled in the same infrastructure
2. Backward compatible: Time tracking is opt-in via running flag
3. Proper cleanup: The bin/compare-bundle-sizes script properly cleans up the modified config file
4. Clear documentation: The script help text is well-written
5. Consistent with existing patterns: Follows the same approach as bundle size monitoring

📝 Before Merging - Checklist

• Fill out PR description with clear summary
• Add unit tests for new functionality
• Fix git restore in cleanup function
• Update CI workflow comment from "base + 0.5KB" to "size + time"
• Explain why compare command was removed
• Document the 20% threshold reasoning
• Consider enabling time tracking for entries with import
• Test bin/compare-bundle-sizes locally to ensure output is helpful
• Run bundle exec rubocop (per CLAUDE.md requirements)
• Ensure all files end with newline (per CLAUDE.md requirements)

🤔 Questions for Discussion

1. Have you measured the actual variance in execution time across CI runs?
2. Should we enable time tracking immediately or in a follow-up PR?
3. Should the running field be renamed to something more descriptive?
4. Can the 20% threshold be reduced with multiple measurement iterations?

Great work on extending the bundle monitoring infrastructure! The implementation is solid, but please address the critical issues (tests, PR description, git restore bug) before merging.

Code Review: Running Time Check on CI

Thank you for working on this PR! I've reviewed the changes and have several important concerns and suggestions:

🚨 Critical Issues

1. Incomplete PR Description

The PR description still contains placeholder text and hasn't been filled in properly. Please:

• Remove placeholder instructions
• Describe what this PR does (adds execution time monitoring to bundle size checks)
• Mark the checklist items appropriately
• Note: The CHANGELOG has been updated ✅

2. Missing Documentation for running Property

The .size-limit.json file now includes "running": false for all entries, but:

• There's no explanation of what this property does
• The comment in bundle-size.mjs (line 21-22) mentions "the current approach is not accurate enough" but doesn't explain why or what the limitation is
• Users need to understand when to set running: true vs running: false

Recommendation: Add documentation explaining:

• What the running property controls (execution time measurement)
• Why it's set to false for all current entries
• When/how to enable it (running: true)
• What "20% threshold" means in practice

3. Typo in Comments

Line 21 of scripts/bundle-size.mjs:

// 20% is a big ration, but the current approach...

Should be: "ratio" not "ration"

4. Missing Test Coverage

According to CLAUDE.md guidelines:

• The PR checklist shows tests are not added
• Changes to build/CI infrastructure should be tested locally
• No evidence of manual testing with bin/compare-bundle-sizes

Critical Test: Did you test bin/compare-bundle-sizes locally with entries that have "running": true? This is the main feature being added.

⚠️ Design Concerns

5. git restore .size-limit.json in Cleanup

Line 28 of bin/compare-bundle-sizes now includes:

git restore .size-limit.json

Questions:

• Why is this needed? The script should leave the working directory clean via the existing cleanup logic
• This could discard intentional local changes to .size-limit.json
• Was there a specific bug this fixes? If so, document it

Recommendation: Either remove this (if unnecessary) or add a comment explaining why it's needed.

6. Removed Comparison Logic Without Explanation

The PR removes 158 lines from bundle-size.mjs, including the entire compare command with its detailed reporting. The new approach:

• Relies entirely on size-limit's built-in comparison
• Loses the custom formatted output that was more user-friendly
• No explanation for why this trade-off was made

Questions:

• Does size-limit's output show execution time comparisons clearly?
• Does it show the 20% threshold violation messages?
• Should we keep some custom reporting for execution time specifically?

✅ Positive Aspects

1. Good changelog entry - Clear description of the feature
2. Simplified architecture - Using size-limit's native features more directly
3. Reasonable defaults - 20% threshold seems appropriate for initial implementation
4. Updated bin/compare-bundle-sizes - Correctly adapted to new workflow

📋 Action Items

Before merging, please:

1. ✏️ Fill in the PR description properly
2. 📝 Add documentation for the running property (in CONTRIBUTING.md or code comments)
3. 🐛 Fix the "ration" → "ratio" typo
4. 🧪 TEST LOCALLY:

   # Create a test entry with running: true
   # Modify .size-limit.json temporarily
   bin/compare-bundle-sizes
   # Verify it measures and compares execution time

5. 🤔 Clarify why git restore .size-limit.json is needed in cleanup
6. 📊 Verify size-limit's output clearly shows time comparison results
7. ✅ Update the PR checklist

🔍 Testing Recommendation

Per CLAUDE.md critical requirements:

NEVER claim a test is "fixed" without running it locally first

Please run these commands and share the output:

# Test with current config (all running: false)
bin/compare-bundle-sizes

# Test with running: true for one entry
# (manually edit .size-limit.json first)
bin/compare-bundle-sizes

Overall Assessment

The core idea is sound, but the PR needs polish before merging:

• Code quality: Good, but needs typo fix
• Documentation: Insufficient
• Testing: Not demonstrated
• PR hygiene: Needs improvement

Happy to re-review once these items are addressed! 🚀

PR Review

Summary

This is a draft PR with an empty commit and no code changes. The PR title is "Add running time check on CI" but currently contains no implementation.

Current State

• Status: Draft (expected for work-in-progress)
• Changes: Empty commit with 0 file changes
• Base branch: add-running-time-check-on-ci-base (feature branch, not master)
• PR description: Template text hasn't been filled in yet

Recommendations

1. Complete the Implementation

The PR title suggests adding a running time check on CI, but no code has been added yet. Consider implementing:

• CI workflow modifications to track job duration
• Alerting or reporting mechanisms for long-running jobs
• Documentation of acceptable time thresholds
• Consider using GitHub Actions built-in timing features

2. Update PR Description

Replace the template text with:

• Summary: Clear description of what the running time check does and why it's needed
• Implementation details: How the check works
• Benefits: What problem this solves
• Testing approach: How you verified it works

3. PR Checklist Items

Based on CLAUDE.md guidelines, ensure you:

• Run bundle exec rubocop and fix all violations before final commit
• Ensure all files end with newline characters
• Run relevant tests locally before pushing
• Update documentation if this affects contributor workflow

4. Base Branch Consideration

This PR targets add-running-time-check-on-ci-base instead of master. Is this intentional? If this is part of a stacked PR workflow, that's fine. If not, consider retargeting to master once ready.

5. Suggested Implementation

For adding running time checks on CI, consider using GitHub Actions timeout-minutes feature or add custom duration tracking with warnings for long-running jobs.

Next Steps

1. Implement the running time check functionality
2. Fill in the PR description with meaningful details
3. Test the changes locally or in a test workflow
4. Mark as ready for review when complete

Review by Claude Code following CLAUDE.md guidelines

PR Review: Add running time check on CI

Current Status

This PR is currently a draft with no visible changes yet (0 additions, 0 deletions, 0 files changed).

Recommendations for Implementation

Based on the title "Add running time check on CI" and the project's guidelines in CLAUDE.md, here are suggestions for when you implement this feature:

1. Scope and Purpose

• Clarify the goal: What specific running time issue are you trying to detect?
  • CI jobs exceeding time limits?
  • Performance regression detection?
  • Identifying slow tests?

2. Implementation Considerations

If adding timeout checks to workflows:

# Example for .github/workflows/*.yml
jobs:
  test:
    timeout-minutes: 30  # Set appropriate limits

If adding performance monitoring:

• Consider using GitHub Actions' built-in time command
• Store historical data for comparison
• Set thresholds for acceptable run times

3. Testing Requirements (per CLAUDE.md)

Before marking this PR as ready for review:

• Test the changes locally if possible
• Update the PR description with:
  • What problem this solves
  • How you tested it
  • Expected behavior
• Add relevant tests if applicable
• Update CHANGELOG.md if this is a user-visible feature
• Ensure bundle exec rubocop passes with zero offenses

4. Files to Consider Modifying

Based on similar features in the codebase:

• .github/workflows/*.yml - Add timeout settings or timing checks
• Potentially add a new script in scripts/ if complex logic is needed
• CLAUDE.md or .claude/docs/ - Document the new CI behavior

5. Best Practices

• Master branch health: Ensure this doesn't introduce flaky failures
• Clear error messages: If a timeout occurs, make it obvious why
• Configurable: Consider making time limits configurable
• Documentation: Update relevant docs about CI behavior

6. CI Failure Monitoring

Per .claude/docs/master-health-monitoring.md:

• After merging, monitor CI for 30 minutes
• Use bin/ci-rerun-failures to verify no regressions
• Poll every 30 seconds (not 180) for responsive feedback

Next Steps

1. Implement the feature
2. Update the PR description with details
3. Mark as ready for review when complete
4. Respond to this comment with context about what you're adding

Looking forward to seeing the implementation! Feel free to ask questions if you need guidance on any specific aspect.

Code Review

Thank you for your contribution! I've reviewed PR #2160 and have the following feedback:

🚨 Critical Issues

1. Incomplete PR Description

   • The PR body still contains placeholder text that should be removed
   • The title mentions "Add running time check on CI" but the changes don't reflect this
   • Please provide a clear description of what this PR is trying to accomplish

2. Test/Temporary File Committed

   • packages/react-on-rails-pro/temp.js appears to be a test/temporary file
   • This file should not be committed to the repository
   • Temporary files like this should be in .gitignore or removed before PR submission

3. PR Checklist Not Completed

   • Tests: No tests added for any new functionality
   • Documentation: Not updated
   • CHANGELOG: Not updated (though this may not need an entry if it's infrastructure)

📋 Questions

1. What is the actual goal?

   • If this is to test CI timing, please explain in the PR description
   • If this is a work-in-progress, consider marking it as a draft PR
   • If this was accidentally opened, it should be closed

2. Missing Implementation

   • The title suggests CI timing checks, but there are no workflow modifications
   • No .github/workflows files were changed
   • No timing measurement code was added

🎯 Recommendations

If this is a legitimate PR:

1. Remove packages/react-on-rails-pro/temp.js
2. Add actual CI timing check implementation in workflow files
3. Update PR description with clear goals and implementation details
4. Complete the PR checklist

If this is a test/experiment:

1. Close this PR
2. Use a draft PR for experimental work
3. Ensure temporary files are never committed

📚 Repository Guidelines

According to CLAUDE.md:

• ALWAYS run bundle exec rubocop before committing (for Ruby files)
• ALWAYS ensure files end with a newline character
• Never commit temporary or test files to the repository
• Update CHANGELOG for user-visible changes only

🔍 Additional Notes

The Pro package (packages/react-on-rails-pro/) has sensitive code and should be handled carefully. Unnecessary files increase repository size and can cause confusion.

Verdict: This PR cannot be merged in its current state. Please clarify the intent and either implement the actual functionality or close the PR if it was opened accidentally.

Let me know if you need help implementing CI timing checks or have questions about the repository structure!

size-limit report 📦

Code Review for PR #2160: Add running time check on CI

Summary

This PR enables the running time check option in .size-limit.json and adds a temporary test file. The running option measures JavaScript execution time in addition to bundle size when using the @size-limit/time plugin.

Issues Found

🚨 CRITICAL: Temporary test file committed

packages/react-on-rails-pro/temp.js

Problem: This appears to be a temporary debugging/test file that should not be committed to the repository.

Action Required: Remove this file before merging:

git rm packages/react-on-rails-pro/temp.js

⚠️ Missing PR Description

Problem: The PR body still contains placeholder text and doesn't explain:

• Why running time checks are being enabled
• What problem this solves
• What performance impact to expect (running time checks add overhead to CI)

Action Required: Update the PR description to explain:

1. Purpose: Why add execution time measurement?
2. Trade-offs: Running time checks add ~10-30s to CI runs
3. Expected behavior: What will happen if code slows down execution time?

📋 PR Checklist Not Completed

The PR checklist items are unchecked:

• Add/update test to cover these changes
• Update documentation
• Update CHANGELOG file

Clarification Needed:

1. Tests: Does this change need tests? If not, explain why in the PR description.

2. Documentation: Should there be documentation about size-limit config? If not, explain.

3. CHANGELOG: Based on CLAUDE.md, this is an infrastructure/tooling change. Per the guidelines:

   Do NOT add entries for: linting, formatting, refactoring, tests, or documentation fixes

   If this is purely CI infrastructure, it may not need a CHANGELOG entry. However, if it affects contributors (e.g., local pnpm run size now checks execution time), document it.

Technical Considerations

📊 running: true Option Impact

According to size-limit documentation, running: true enables the @size-limit/time plugin which:

• Measures JavaScript execution time in a headless browser
• Adds significant overhead to CI runs (10-30+ seconds per package checked)
• Requires @size-limit/time package (✅ already installed in package.json:25)

Questions:

1. Performance Impact: Have you measured how much this increases CI time?
2. Baseline Establishment: What are the current execution times for all 8 checks?
3. Thresholds: Are there time limits configured? (I don't see maxTime in the config)

🔧 Configuration Completeness

Current config only has running: true but no time limits:

{
  "name": "react-on-rails/client bundled (gzip)",
  "path": "packages/react-on-rails/lib/ReactOnRails.client.js",
  "import": "ReactOnRails",
  "gzip": true,
  "running": true  // ✅ Enabled but...
  // ❓ Missing: "maxTime": "1s"
}

Recommendation: Consider adding maxTime limits to make this actionable:

{
  "name": "react-on-rails/client bundled (gzip)",
  "path": "packages/react-on-rails/lib/ReactOnRails.client.js",
  "import": "ReactOnRails",
  "gzip": true,
  "running": true,
  "maxTime": "500ms"  // Set appropriate threshold
}

Without maxTime, the checks will run but won't fail on regressions.

Testing Requirements (Per CLAUDE.md)

From the project guidelines:

CRITICAL - LOCAL TESTING REQUIREMENTS:

1. NEVER claim a test is "fixed" without running it locally first
2. Distinguish hypothetical fixes from confirmed fixes

Required Testing:

# 1. Test size-limit locally with running checks
pnpm run build
pnpm run size

# 2. Verify output includes execution time measurements
# Expected: Should show both size AND running time for each entry

# 3. Test CI workflow locally (optional but recommended)
# This validates the full workflow including dynamic limits

Have you verified locally that:

1. The running checks execute successfully?
2. Execution times are reasonable (not excessively slow)?
3. The CI workflow still works with running: true?

Recommendations

✅ Before Merging:

1. Remove temp.js file
2. Fill out PR description explaining the purpose and trade-offs
3. Test locally: Run pnpm run size and verify output
4. Document baselines: Post the current execution times in the PR for reference
5. Consider adding maxTime limits to make checks actionable
6. Update checklist or explain why items don't apply

🎯 Optional Improvements:

1. Staggered Rollout: Consider enabling running on just 1-2 packages first to assess impact
2. Documentation: Add a comment in .size-limit.json explaining the running option
3. CI Monitoring: Track how much this increases bundle-size workflow duration

Security & Performance

• ✅ No security concerns
• ⚠️ Performance impact: Running time checks add overhead to CI (measure and document)
• ✅ No breaking changes

Final Verdict

Status: NEEDS WORK ❌

Blocking Issues:

1. Remove temp.js file
2. Complete PR description
3. Verify local testing completed

Non-Blocking Suggestions:

• Add maxTime limits for actionable checks
• Document execution time baselines
• Consider staggered rollout

Note: This is a draft PR, so these issues are expected. Address the blocking items before marking as ready for review.