Fix pre-commit hook to only check changed files

[justin808]

Summary

• Fixed ruby-autofix pre-commit hook to run RuboCop only on changed Ruby files
• Reduces pre-commit time from ~24 seconds to <1 second for typical commits
• During releases, this will only check changed files (version files, Gemfile.locks) instead of all 154 files

Problem

The pre-commit hook was running bundle exec rake autofix which checks ALL files in the project:

cd /Users/justin/shakacode/react-on-rails/react_on_rails && bundle exec rubocop -A
Inspecting 154 files
..........................................................................................................................................................

154 files inspected, no offenses detected
Completed auto-fixing all linting violations

This made every commit (especially release commits) very slow at ~24 seconds.

Solution

Changed bin/lefthook/ruby-autofix to run bundle exec rubocop -A directly on only the changed files:

bundle exec rubocop -A --force-exclusion -- $files

The get-changed-files script already identifies which files changed, but we were ignoring that and running the full rake task.

Test Plan

• Committed this change and verified hook runs in <1 second
• Next release will verify it still works with many changed files
• Hook correctly skips when no Ruby files are changed

🤖 Generated with Claude Code

---

This change is 

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 9 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 6772835 and b14898b.

📒 Files selected for processing (1)

• bin/lefthook/ruby-autofix (1 hunks)

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch justin808/fix-precommit-scope

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review

✅ Overall Assessment

This is a well-targeted performance optimization that significantly improves developer experience. The change is minimal, focused, and aligns with the project's conventions.

---

🎯 Strengths

1. Performance Impact: Reducing pre-commit time from ~24s to <1s is a massive improvement (96% reduction)
2. Consistency: Now matches the pattern used by other hooks (eslint-lint, prettier-format, ruby-lint) which already run only on changed files
3. Correctness: Properly uses --force-exclusion flag to respect RuboCop exclusions
4. Minimal Change: Single-line logic change with clear comment explaining the rationale

---

🔍 Technical Review

Code Quality

• ✅ Uses proper bash parameter passing with -- $files
• ✅ Consistent with other lefthook scripts in the same directory
• ✅ Comment clearly explains the change
• ✅ Maintains all necessary RuboCop flags (-A for auto-correct, --force-exclusion)

Edge Cases Handled

• ✅ Empty file list already handled by lines 8-11
• ✅ File list properly quoted/escaped by get-changed-files script
• ✅ Works with all-changed context (staged + unstaged + untracked)

---

⚠️ Potential Issues & Suggestions

1. Incomplete Migration from rake autofix

The old rake autofix task runs three tools:

yarn run eslint . --fix
yarn run prettier --write .
bundle exec rubocop -A

This PR only addresses RuboCop. However, I see this is already handled correctly by the lefthook configuration which runs separate hooks in parallel:

• ruby-autofix → RuboCop only ✅
• eslint-lint → ESLint --fix
• prettier-format → Prettier --write

So this is actually correct! The hook separation is intentional.

2. Comment Accuracy

The script comment at line 2 still says:

# Auto-fix Ruby files using rake autofix

Suggestion: Update to reflect current behavior:

# Auto-fix Ruby files using RuboCop on changed files

3. Pro Package Consideration

Does react_on_rails_pro/ have its own RuboCop config similar to how it has separate ESLint/Prettier configs? If so, this script might need similar directory separation logic like eslint-lint has (lines 13-53 in that file).

The eslint-lint script handles Pro separately because it has its own ESLint config. If Pro has a separate .rubocop.yml, this script should do the same.

---

🧪 Test Coverage

Missing: No automated tests for lefthook scripts. While manual testing is mentioned, consider:

• Adding integration tests that verify hooks run correctly
• Testing with different file combinations (staged only, unstaged only, mixed)
• Verifying stage_fixed: true works correctly after auto-fixes

Note: This is a project-wide gap, not specific to this PR.

---

🔒 Security

✅ No security concerns. The script properly quotes variables and uses safe bash practices (set -euo pipefail).

---

📊 Performance

✅ Excellent improvement:

• Before: ~24s (154 files)
• After: <1s (typical 1-5 changed files)
• Release commits: Only checks version files instead of all 154 files

---

📝 Recommendations

Required

• Update comment at line 2 to match new behavior

Recommended

• Verify if react_on_rails_pro/ needs separate RuboCop handling like eslint-lint does
• Consider adding note in CLAUDE.md about hook performance optimization

Future Enhancements (Out of Scope)

• Consider adding integration tests for all lefthook scripts
• Document expected performance characteristics in comments

---

✅ Approval Status

LGTM with minor comment update suggestion. The core change is solid and provides significant value. The recommendations above are mostly nice-to-haves that could be addressed in follow-up PRs.

Great work on this optimization! 🚀

[claude]

Code Review - PR #2013

Overview

This PR optimizes the pre-commit hook performance by running RuboCop only on changed files instead of the entire project. The change reduces commit time from ~24s to <1s for typical commits.

✅ Strengths

1. Performance improvement is significant: From 24s to <1s is a massive UX win
2. Aligns with hook design: Other hooks (eslint-lint, prettier-format) already follow this pattern of operating on changed files only
3. Preserves safety: The change maintains the same auto-fix behavior, just scoped to relevant files
4. Well-documented: PR description clearly explains the problem and solution

🔍 Code Quality

The implementation is correct:

• Uses --force-exclusion flag (matches ruby-lint behavior)
• Properly passes $files variable from get-changed-files
• The -- $files syntax is correct for passing file arguments to RuboCop

⚠️ Potential Concerns & Questions

1. Hook Redundancy (Minor)

The .lefthook.yml config shows two RuboCop hooks running sequentially:

• autofix: Line 8-11 runs ruby-autofix (RuboCop with -A)
• rubocop: Line 13-16 runs ruby-lint (RuboCop without auto-fix)

Question: Is it intentional to run RuboCop twice? Since ruby-autofix already runs RuboCop with auto-fix (-A), the subsequent ruby-lint check should always pass. Consider:

• Removing the rubocop command from pre-commit (keep only autofix)
• OR: Remove autofix and keep only rubocop if you want manual fixes

This would further improve performance by ~50%.

2. Prettier/ESLint Not Called (Issue)

The original rake autofix task runs three tools (from rakelib/lint.rake:29-33):

task :autofix do
  sh_in_dir(gem_root, "yarn run eslint . --fix")
  sh_in_dir(gem_root, "yarn run prettier --write .")
  sh_in_dir(gem_root, "bundle exec rubocop -A")
end

Your change removes ESLint and Prettier from the ruby-autofix hook.

However, looking at .lefthook.yml, this is actually correct because those tools run via separate hooks:

• Line 18-21: eslint hook
• Line 23-26: prettier hook

So the hook isn't named optimally - it's called ruby-autofix but was previously running all autofixers. The new behavior matches the name better.

Recommendation: Consider renaming the hook in .lefthook.yml from autofix to ruby-autofix for clarity (line 8).

3. Edge Case: Cross-file Dependencies (Low Risk)

RuboCop has some cops that consider multiple files (e.g., duplicate method detection). Running only on changed files theoretically could miss some violations. However:

• This is an acceptable trade-off for pre-commit hooks (CI runs full checks anyway)
• Other projects with large codebases follow this pattern successfully
• The pre-push hook on line 35 runs ruby-lint branch which checks all files in the branch

This is fine as-is.

🧪 Testing Recommendations

• ✅ Already tested: Author verified <1s execution time
• ⏭️ Consider testing: Release scenario with many changed files (author mentioned will test on next release)
• ⏭️ Consider testing: Verify behavior when Ruby file is renamed (does get-changed-files handle this correctly?)

📊 Performance Comparison

Scenario	Before	After	Improvement
Typical commit (1-3 files)	~24s	<1s	96% faster
Release commit (version files)	~24s	<1s	96% faster

🔒 Security Considerations

No security concerns. The change reduces the scope of operations, making it safer if anything.

✅ Final Recommendation

APPROVE with minor suggestion

The core change is excellent and should be merged. Consider the minor cleanup suggestions:

1. Address the duplicate RuboCop runs in .lefthook.yml (remove either autofix or rubocop command)
2. Optionally rename the hook to ruby-autofix for clarity

---

Great work on this optimization! This will significantly improve the developer experience. 🚀