Add lefthook #842
Add lefthook #842
Conversation
|
Warning Rate limit exceeded@Seifeldin7 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (3)
WalkthroughReplaces Husky + lint-staged with Lefthook: removes Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Git
participant Lefthook
participant Script as bin/lefthook/*
participant Tool as Linters/Tests
Dev->>Git: git commit
Git->>Lefthook: trigger pre-commit
Lefthook->>Lefthook: read lefthook.yml
par Parallel hooks
Lefthook->>Script: rubocop-lint (rb files)
Script->>Tool: bundle exec rubocop --autocorrect-all
Tool-->>Script: result
Lefthook->>Script: js-lint (js/ts files)
Script->>Tool: eslint --fix / prettier --write / tsc checks
Tool-->>Script: result
Lefthook->>Script: prettier-format (staged)
Script->>Tool: yarn prettier --write
Tool-->>Script: result
Lefthook->>Script: rspec-affected / jest-affected
Script->>Tool: bundle exec rspec / yarn test (affected files)
Tool-->>Script: result
Lefthook->>Script: check-trailing-newlines
Script-->>Script: append missing newlines
end
Lefthook->>Git: aggregated exit code
alt all pass
Git->>Dev: commit proceeds
else any fail
Git->>Dev: commit blocked (show failures)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: Lefthook MigrationOverall AssessmentThis is a well-executed migration from Husky + lint-staged to Lefthook. The implementation is clean, comprehensive, and includes excellent documentation. The PR demonstrates good engineering practices with proper cleanup, feature parity, and enhanced capabilities. ✅ Strengths1. Clean Migration Strategy
2. Well-Structured Hook Scripts
3. Enhanced Functionality
4. Excellent Documentation
5. Performance Benefits
🐛 Issues Found1. Critical: Unquoted Variables in Shell Scripts (Security & Correctness)Multiple scripts have unquoted variable expansions that will break with filenames containing spaces or special characters: bin/lefthook/rubocop-lint:14 bundle exec rubocop --autocorrect-all $@bin/lefthook/js-lint:35, 41 yarn eslint --fix $format_files || has_errors=1
yarn prettier --write $format_files || has_errors=1bin/lefthook/prettier-format:14 yarn prettier --write $@bin/lefthook/rspec-affected:40 bundle exec rspec $spec_filesbin/lefthook/jest-affected:64 yarn test --findRelatedTests $test_filesFix: Quote all variable expansions or use arrays for accumulating filenames. 2. Issue: check-trailing-newlines Has Side Effectsbin/lefthook/check-trailing-newlines:25 This script modifies files but exits with code 1, which will:
Implications:
3. Potential Issue: TypeScript Type Check Script Dependencybin/lefthook/js-lint:47 This assumes Recommendation: Add existence check before running. 4. Minor: Inconsistent Error Handling PatternSome scripts check exit codes immediately, while others accumulate errors. The accumulated error pattern is better for running multiple checks, but mixing patterns reduces consistency.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CONTRIBUTING.md (1)
34-52: Align remaining setup instructions with Lefthook (remove Husky reference)The new “Git Hooks (Optional)” section correctly documents Lefthook (
bundle exec lefthook install), but the later dev‑env setup still says:bundle install yarn install yarn prepare:husky # Set up pre-commit hooks for lintingThis is now stale and will mislead contributors.
Consider updating that block to reference Lefthook instead (or to point back to the “Git Hooks (Optional)” section), e.g.:
- yarn prepare:husky # Set up pre-commit hooks for linting + bundle exec lefthook install # (Optional) Set up git hooks for linting/testsAlso applies to: 179-184
🧹 Nitpick comments (1)
bin/lefthook/js-lint (1)
4-4: Consider adjusting error handling approach.The script uses
set -ebut then captures errors with|| has_errors=1throughout. While this works, it's potentially confusing sinceset -eis effectively neutralized by the error capture pattern. Consider either removingset -eor usingset +ebefore error-captured commands for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.husky/pre-commit(0 hunks)CONTRIBUTING.md(2 hunks)Gemfile.development_dependencies(1 hunks)bin/lefthook/check-trailing-newlines(1 hunks)bin/lefthook/jest-affected(1 hunks)bin/lefthook/js-lint(1 hunks)bin/lefthook/prettier-format(1 hunks)bin/lefthook/rspec-affected(1 hunks)bin/lefthook/rubocop-lint(1 hunks)conductor-setup.sh(1 hunks)lefthook.yml(1 hunks)package.json(0 hunks)
💤 Files with no reviewable changes (2)
- package.json
- .husky/pre-commit
⏰ 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). (20)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Testing (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.6.1.x)
- GitHub Check: Testing (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.6.1.x)
- GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: test
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.6.1.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.6.0.x)
- GitHub Check: claude-review
🔇 Additional comments (4)
Gemfile.development_dependencies (1)
17-19: LGTM: Lefthook wired in as a dev‑only dependencyUsing a dedicated
:developmentgroup withrequire: falseis a clean way to add Lefthook without impacting runtime or test environments.bin/lefthook/check-trailing-newlines (1)
1-39: LGTM: trailing newline fixer is simple and effectiveThe script correctly skips non-existent/empty files, auto-adds missing trailing newlines, and exits non‑zero to force re‑staging when it makes changes. This is a good fit for a pre‑commit hook.
lefthook.yml (1)
27-37: LGTM!The pre-push configuration appropriately runs full repository checks without glob filtering, ensuring comprehensive validation before pushing changes.
bin/lefthook/js-lint (1)
44-48: No issues found—type checking behavior is intentional and well-documented.The script deliberately ignores file arguments passed by lint-staged and runs full project type checking via
tsc --noEmit. This is by design: as the script's own comments explain, passing individual file paths would cause TypeScript to ignoretsconfig.jsonand fail type checking. The current implementation is correct and follows best practices for TypeScript type checking in a lint-staged workflow.Likely an incorrect or invalid review comment.
bin/lefthook/js-lint
Outdated
| # Separate TypeScript files from JavaScript files | ||
| ts_files="" | ||
| js_files="" | ||
| format_files="" | ||
|
|
||
| for file in "$@"; do | ||
| # Add all files to format_files for prettier | ||
| format_files="$format_files $file" | ||
|
|
||
| # Separate TS/TSX from JS/JSX for type checking | ||
| if [[ "$file" == *.ts || "$file" == *.tsx ]]; then | ||
| ts_files="$ts_files $file" | ||
| elif [[ "$file" == *.js || "$file" == *.jsx ]]; then | ||
| js_files="$js_files $file" | ||
| fi | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use bash arrays instead of string concatenation for file lists.
Building file lists via string concatenation (ts_files="$ts_files $file") is fragile and will break when filenames contain spaces or special characters. Bash arrays provide proper handling and quoting.
Apply this refactor to use arrays:
# Separate TypeScript files from JavaScript files
-ts_files=""
-js_files=""
-format_files=""
+ts_files=()
+js_files=()
+format_files=()
for file in "$@"; do
# Add all files to format_files for prettier
- format_files="$format_files $file"
+ format_files+=("$file")
# Separate TS/TSX from JS/JSX for type checking
if [[ "$file" == *.ts || "$file" == *.tsx ]]; then
- ts_files="$ts_files $file"
+ ts_files+=("$file")
elif [[ "$file" == *.js || "$file" == *.jsx ]]; then
- js_files="$js_files $file"
+ js_files+=("$file")
fi
doneThen update the command invocations to use array expansion:
# Run ESLint with --fix on JS/TS files
-if [ -n "$js_files" ] || [ -n "$ts_files" ]; then
+if [ ${#js_files[@]} -gt 0 ] || [ ${#ts_files[@]} -gt 0 ]; then
echo "Running ESLint --fix..."
- yarn eslint --fix $format_files || has_errors=1
+ yarn eslint --fix "${format_files[@]}" || has_errors=1
fi
# Run Prettier on all files
-if [ -n "$format_files" ]; then
+if [ ${#format_files[@]} -gt 0 ]; then
echo "Running Prettier..."
- yarn prettier --write $format_files || has_errors=1
+ yarn prettier --write "${format_files[@]}" || has_errors=1
fi
# Run TypeScript type checking on TS/TSX files
-if [ -n "$ts_files" ]; then
+if [ ${#ts_files[@]} -gt 0 ]; then
echo "Running TypeScript type checking..."
node scripts/type-check-no-emit.js || has_errors=1
fiCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bin/lefthook/prettier-format (1)
14-14: Quote$@to handle filenames with spaces.While the
set -eissue from the previous review has been addressed,$@should still be quoted as"$@"to properly handle filenames containing spaces or special characters.Apply this diff:
-if ! yarn prettier --write $@; then +if ! yarn prettier --write "$@"; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CONTRIBUTING.md(3 hunks)bin/lefthook/jest-affected(1 hunks)bin/lefthook/js-lint(1 hunks)bin/lefthook/prettier-format(1 hunks)bin/lefthook/rspec-affected(1 hunks)lefthook.yml(1 hunks)spec/dummy/bin/shakapacker-config(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- spec/dummy/bin/shakapacker-config
🚧 Files skipped from review as they are similar to previous changes (4)
- bin/lefthook/js-lint
- lefthook.yml
- bin/lefthook/jest-affected
- CONTRIBUTING.md
⏰ 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). (20)
- GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Testing (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.6.0.x)
- GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.6.1.x)
- GitHub Check: test
- GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: claude-review
| spec_files="" | ||
| for file in "$@"; do | ||
| # Skip if file is already a spec file | ||
| if [[ "$file" == *"_spec.rb" ]]; then | ||
| if [ -f "$file" ]; then | ||
| spec_files="$spec_files $file" | ||
| fi | ||
| continue | ||
| fi | ||
|
|
||
| # Convert lib/shakapacker/foo.rb to spec/shakapacker/foo_spec.rb | ||
| if [[ "$file" == lib/* ]]; then | ||
| spec_file="${file/lib\//spec/}" | ||
| spec_file="${spec_file/.rb/_spec.rb}" | ||
| if [ -f "$spec_file" ]; then | ||
| spec_files="$spec_files $spec_file" | ||
| fi | ||
| fi | ||
| done | ||
|
|
||
| if [ -z "$spec_files" ]; then | ||
| echo "No corresponding spec files found for staged files" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "Running RSpec for affected files..." | ||
| echo "Spec files: $spec_files" | ||
|
|
||
| if ! bundle exec rspec $spec_files; then | ||
| echo "" | ||
| echo "Tests failed. Please fix the issues before committing." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Use array instead of space-separated string for spec files.
While the set -e issue from the previous review has been addressed, the script still uses string concatenation with spaces to build spec_files (lines 17, 27). This approach breaks when spec file paths contain spaces or special characters. At line 40, the unquoted $spec_files will cause incorrect word-splitting.
Apply this diff to use a bash array:
# Convert source files to their corresponding spec files
-spec_files=""
+spec_files=()
for file in "$@"; do
# Skip if file is already a spec file
if [[ "$file" == *"_spec.rb" ]]; then
if [ -f "$file" ]; then
- spec_files="$spec_files $file"
+ spec_files+=("$file")
fi
continue
fi
# Convert lib/shakapacker/foo.rb to spec/shakapacker/foo_spec.rb
if [[ "$file" == lib/* ]]; then
spec_file="${file/lib\//spec/}"
spec_file="${spec_file/.rb/_spec.rb}"
if [ -f "$spec_file" ]; then
- spec_files="$spec_files $spec_file"
+ spec_files+=("$spec_file")
fi
fi
done
-if [ -z "$spec_files" ]; then
+if [ ${#spec_files[@]} -eq 0 ]; then
echo "No corresponding spec files found for staged files"
exit 0
fi
echo "Running RSpec for affected files..."
-echo "Spec files: $spec_files"
+echo "Spec files: ${spec_files[*]}"
-if ! bundle exec rspec $spec_files; then
+if ! bundle exec rspec "${spec_files[@]}"; then
echo ""
echo "Tests failed. Please fix the issues before committing."
exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spec_files="" | |
| for file in "$@"; do | |
| # Skip if file is already a spec file | |
| if [[ "$file" == *"_spec.rb" ]]; then | |
| if [ -f "$file" ]; then | |
| spec_files="$spec_files $file" | |
| fi | |
| continue | |
| fi | |
| # Convert lib/shakapacker/foo.rb to spec/shakapacker/foo_spec.rb | |
| if [[ "$file" == lib/* ]]; then | |
| spec_file="${file/lib\//spec/}" | |
| spec_file="${spec_file/.rb/_spec.rb}" | |
| if [ -f "$spec_file" ]; then | |
| spec_files="$spec_files $spec_file" | |
| fi | |
| fi | |
| done | |
| if [ -z "$spec_files" ]; then | |
| echo "No corresponding spec files found for staged files" | |
| exit 0 | |
| fi | |
| echo "Running RSpec for affected files..." | |
| echo "Spec files: $spec_files" | |
| if ! bundle exec rspec $spec_files; then | |
| echo "" | |
| echo "Tests failed. Please fix the issues before committing." | |
| exit 1 | |
| fi | |
| spec_files=() | |
| for file in "$@"; do | |
| # Skip if file is already a spec file | |
| if [[ "$file" == *"_spec.rb" ]]; then | |
| if [ -f "$file" ]; then | |
| spec_files+=("$file") | |
| fi | |
| continue | |
| fi | |
| # Convert lib/shakapacker/foo.rb to spec/shakapacker/foo_spec.rb | |
| if [[ "$file" == lib/* ]]; then | |
| spec_file="${file/lib\//spec/}" | |
| spec_file="${spec_file/.rb/_spec.rb}" | |
| if [ -f "$spec_file" ]; then | |
| spec_files+=("$spec_file") | |
| fi | |
| fi | |
| done | |
| if [ ${#spec_files[@]} -eq 0 ]; then | |
| echo "No corresponding spec files found for staged files" | |
| exit 0 | |
| fi | |
| echo "Running RSpec for affected files..." | |
| echo "Spec files: ${spec_files[*]}" | |
| if ! bundle exec rspec "${spec_files[@]}"; then | |
| echo "" | |
| echo "Tests failed. Please fix the issues before committing." | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In bin/lefthook/rspec-affected around lines 12 to 44, the script builds
spec_files as a space-separated string which breaks on paths with spaces and is
later unquoted when invoked; change spec_files to a bash array (initialize
spec_files=()), append entries with spec_files+=("$file") where the script
currently does string concatenation (lines ~17 and ~27), check emptiness with [
${#spec_files[@]} -eq 0 ] (instead of -z), and invoke RSpec with properly quoted
array expansion: bundle exec rspec "${spec_files[@]}"; also update any echo of
the list to handle the array (e.g., echo "Spec files: ${spec_files[*]}").
Pull Request Review: Lefthook MigrationOverall AssessmentThis is a well-executed migration from Husky + lint-staged to Lefthook. The implementation is thorough, well-documented, and provides excellent feature parity plus enhancements. The code quality is high with good error handling and user feedback. Strengths1. Excellent Documentation
2. Feature Enhancement
3. Clean Architecture
4. Good Error Handling
Issues Found1. CRITICAL: Missing set -e in js-lint script (bin/lefthook/js-lint:4)Problem: Unlike the other scripts, js-lint does not have set -e at the top, which means it will not exit on command failures. It relies on manual error tracking with has_errors, but this is less robust. Impact: Could mask errors if commands fail unexpectedly. Fix: Either add set -e and refactor to use explicit error handling similar to other scripts, OR intentionally omit it and document why this script uses a different error handling approach. 2. Potential Issue: Unquoted Variable Expansion (Multiple files)All scripts use unquoted variable expansions which can cause issues with filenames containing spaces:
Problem: File paths with spaces will be split into multiple arguments. Impact: Scripts will fail on files with spaces in their names. Fix: Quote all variable expansions throughout all scripts. 3. Bug: Variable naming inconsistency (bin/lefthook/js-lint:31-33)The condition checks $js_files and $ts_files but the command uses $format_files which includes ALL files: if [ -n "$js_files" ] || [ -n "$ts_files" ]; then
echo "Running ESLint --fix..."
yarn eslint --fix $format_files || has_errors=1
fiClarification needed: Should ESLint run only on js/ts files or all files? The current logic is confusing. Performance Considerations1. Pre-commit hooks might be too aggressiveRunning BOTH linting/formatting AND tests on pre-commit could slow down development workflow significantly. Suggestion: Consider making the test hooks (rspec-affected jest-affected) optional or configurable. Many teams prefer running tests only on pre-push to keep commits fast. Alternative: Add Lefthook environment variable to skip tests: rspec-affected:
run: bin/lefthook/rspec-affected {staged_files}
skip:
- env: LEFTHOOK_SKIP_TESTS2. Pre-push runs full test suiteThe pre-push hook runs the complete test suite which could be slow. Question: Is this intentional? Consider documenting the expected time and how to skip with --no-verify for emergency pushes. Code Quality & Best PracticesGood practices found:
Suggestions:
Test Coverage Questions
RecommendationsMust Fix (Before Merge):
Should Fix:
Nice to Have:
Final VerdictRecommendation: Approve with minor changes This is a high-quality PR that significantly improves the projects git hooks infrastructure. The migration is well-planned and thorough. The critical issues (quoting variables fixing set -e) are straightforward to address. Once those are fixed this is ready to merge. Great work on the comprehensive documentation and test coverage enhancements! |
Code Review: Lefthook MigrationI've reviewed PR #842. Overall, this is a solid improvement to the project's tooling infrastructure, but there are some important issues to address. Critical Issues1. Missing CHANGELOG Entry Action Required: Add a CHANGELOG entry with PR link and author. 2. Shell Script Security: Unquoted Variables
Impact: Files with spaces in names will break these scripts. High Priority Issues3. Incorrect Exit Code Handling in rubocop-lint The script has set -e enabled but tries to capture exit codes manually. The if check at line 17 will never execute because set -e causes immediate exit on error. Fix: Either remove set -e OR use || true pattern. 4. Pre-Push Hooks May Be Too Aggressive Medium Priority Issues5. check-trailing-newlines Auto-Fix Workflow 6. rspec-affected Limited Scope 7. jest-affected Simplified Detection Code Quality Notes
Positive Highlights
Testing ChecklistHave these been tested?
SummaryPlease address:
The migration strategy is sound and hook organization is excellent. With these fixes, this will be a solid enhancement! |
PR Review: Add LefthookThanks for this comprehensive migration from Husky + lint-staged to Lefthook! Overall, this is a well-structured PR that modernizes the git hook setup. Here's my detailed feedback: ✅ Strengths
🐛 Critical Issues1. Shell Quoting Vulnerability (Security)Multiple scripts have unquoted variable expansions that could break with filenames containing spaces: bin/lefthook/js-lint:25,31,42 yarn eslint --fix $js_ts_files # Should be: "$js_ts_files"
yarn prettier --write $js_ts_files # Should be: "$js_ts_files"
git add $js_ts_files # Should be: "$js_ts_files"bin/lefthook/rspec-affected:40 bundle exec rspec $spec_files # Should be: "$spec_files"bin/lefthook/jest-affected:64 yarn test --findRelatedTests $test_files # Should be: "$test_files"This could cause failures or unexpected behavior with files containing spaces or special characters. 2. Inconsistent Script Error Handlingbin/lefthook/js-lint is missing 3. Pre-Push Hooks May Be Too HeavyThe pre-push hooks run the full test suite on every push (lefthook.yml:36,40). This could make pushes very slow, especially for small changes. Consider:
4. TypeScript Type Check Runs on All Fileslefthook.yml:20-22 runs type-checking on the entire codebase even if only one TS file changed. The old lint-staged approach was smarter. Consider using the glob filtering or removing this from pre-commit.
|
Code Review: Lefthook MigrationI've reviewed this PR that migrates from Husky + lint-staged to Lefthook. Overall, this is a well-structured migration with good improvements. Here are my findings: ✅ Strengths
🐛 Potential IssuesCritical: Missing trailing newlines in new filesAccording to CLAUDE.md, "ALWAYS end all files with a trailing newline character." Several new bash scripts appear to be missing this based on the diff:
Please verify all new files end with a newline character. Code Quality Issues1. Quote consistency in -const { run } = require("shakapacker/configExporter")
+const { run } = require('shakapacker/configExporter')This change is unrelated to the Lefthook migration and should likely be in a separate commit or PR. If it's intentional, it should be mentioned in the PR description. 2. In 3. In has_errors=0
yarn eslint --fix $js_ts_files || has_errors=1
yarn prettier --write $js_ts_files || has_errors=1Both commands will run even if the first fails. This is intentional with 4. Variable quoting in loops: for file in "$@"; doThis is correct. However, when building the file lists, you're not quoting: test_files="$test_files $file" # Good
yarn test --findRelatedTests $test_files # Should be quoted: "$test_files"Files with spaces in names could cause issues. Consider using arrays or proper quoting. 5. In if \! grep -q '"test"' package.json; thenThis could match "test" anywhere in package.json, not just in scripts. Consider using if \! jq -e '.scripts.test' package.json > /dev/null 2>&1; then6. In 📊 Performance Considerations
🔒 Security Considerations
🧪 Test Coverage
📝 Documentation
💡 Suggestions
🎯 Overall AssessmentThis is a solid improvement to the project's development workflow. The migration is well-thought-out and the new functionality (Ruby linting, automatic test running) is valuable. The main issues are:
Once these are addressed, this will be an excellent addition to the project. Recommendation: Request changes for the critical items (trailing newlines, changelog), approve after fixes. |
Code Review - PR #842: Add LefthookThis is a well-structured migration from Husky + lint-staged to Lefthook. Overall excellent work! Strengths
Critical Issues1. Missing Trailing Newlines 2. Potential Bug in check-trailing-newlines 3. Unquoted Variable Expansion 4. TypeScript Type Checking Issue 5. Style Inconsistency Recommendations
Testing Needed
Security & Performance
Action Items
Great work on this migration! Lefthook is a solid choice for this project. |
2 participants
Replace Husky + lint-staged with Lefthook for Git Hooks
Summary
This PR migrates the project's Git hooks from Husky + lint-staged to Lefthook, providing a more powerful, faster, and Ruby-native solution for managing pre-commit and pre-push hooks.
Motivation
Changes
✅ Removed
.husky/directory and pre-commit hookhuskyandlint-stagednpm dependencieslint-stagedconfiguration from package.json✨ Added
Lefthook Configuration (
lefthook.yml):Hook Scripts (
bin/lefthook/):rubocop-lint- Auto-fix Ruby style issues on staged filesjs-lint- ESLint + Prettier + TypeScript type checking for JS/TS filesprettier-format- Prettier formatting for JSON/YAML/Markdown filescheck-trailing-newlines- Auto-fix missing trailing newlinesrspec-affected- Run RSpec tests for changed Ruby filesjest-affected- Run Jest tests for changed JS/TS files📝 Updated
CONTRIBUTING.md- Updated setup instructionsconductor-setup.sh- Changed fromnpx huskytobundle exec lefthook installREADME.md- Added comprehensive Lefthook documentationGemfile.development_dependencies- AddedlefthookgemFeature Parity with lint-staged
All previous lint-staged functionality is preserved and enhanced:
File Type | Previous (lint-staged) | New (Lefthook) -- | -- | -- JS/JSX | ESLint --fix + Prettier | ✅ js-lint script TS/TSX | ESLint --fix + Prettier + Type checking | ✅ js-lint script JSON/YAML/MD | Prettier | ✅ prettier-format script Ruby | ❌ Not supported | ✅ rubocop-lint scriptPlus new features:
Pre-commit Hooks (Parallel Execution)
Fast validation on staged files only:
Pre-push Hooks (Comprehensive)
Full validation before pushing:
Installation
Manual Hook Execution
Bypassing Hooks (when necessary)
Testing
Migration Notes
This is a non-breaking change:
bundle install && bundle exec lefthook installSummary by CodeRabbit
Chores
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.