Add CI workflow to check assets:precompile output for failures #2134
Conversation
WalkthroughAdds a CI job and helper script that run Changes
Sequence DiagramsequenceDiagram
actor Trigger as Workflow Trigger
participant GHA as GitHub Actions
participant DC as detect-changes
participant PC as precompile-check
participant Env as Env Setup (Ruby/Node/pnpm)
participant Yalc as yalc (publish & link)
participant Dummy as Dummy Rails App
participant Scanner as validate-precompile-output
Trigger->>GHA: workflow dispatch / push
GHA->>DC: run detect-changes
DC->>DC: evaluate labels/base/ref/PR diffs
DC-->>GHA: outputs (docs_only, run_dummy_tests, has_full_ci_label)
alt Guarded master docs-only push
GHA->>GHA: skip precompile-check
else Run precompile-check
GHA->>PC: start precompile-check
PC->>Env: setup Ruby, Node, pnpm, install deps
Env-->>PC: environment ready
PC->>Yalc: yalc publish && yalc add --link react-on-rails
Yalc-->>PC: package linked
PC->>Dummy: prepare dummy app (install, link)
Dummy-->>PC: ready
PC->>Dummy: run assets:precompile (capture stdout/stderr -> precompile_output.txt)
Dummy-->>PC: precompile_output.txt
PC->>Scanner: invoke script/validate-precompile-output
alt Failure patterns found
Scanner-->>GHA: emit error annotations & exit non-zero
else No failures
Scanner-->>GHA: pass
end
PC->>GHA: upload precompile_output.txt artifact
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30–40 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 for PR #2134: Assets Precompile Check WorkflowI've reviewed this PR against the repository's CLAUDE.md guidelines and best practices. Overall, this is a well-structured addition that addresses a real pain point (issue #2081). Here's my detailed feedback: ✅ Strengths1. Excellent Problem Alignment
2. Consistent with Existing CI Patterns
3. Good Error Detection CoverageThe 8 failure patterns cover realistic scenarios:
4. Helpful Debugging Features
🔍 Issues & Suggestions1. Critical: Logic Error in Duplicate Compilation Detection (Lines 148-159)Current code: WEBPACK_COMPILE_COUNT=$(grep -c "Compiled" precompile_output.txt || true)
if [ "$WEBPACK_COMPILE_COUNT" -gt 1 ]; then
DUPLICATE_COMPILES=$(sort precompile_output.txt | uniq -d | grep -c "Compiled" || true)
if [ "$DUPLICATE_COMPILES" -gt 0 ]; then
echo "::error::FAILURE: Detected duplicate webpack compilations..."Problem: Example that would NOT be detected: Recommended fix: # Count how many times the word "Compiled" appears in the output
WEBPACK_COMPILE_COUNT=$(grep -c "Compiled" precompile_output.txt || true)
# If we see "Compiled" more than once, it's likely a duplicate execution
# (Shakapacker typically only compiles once during assets:precompile)
if [ "$WEBPACK_COMPILE_COUNT" -gt 1 ]; then
echo "::error::FAILURE: Detected $WEBPACK_COMPILE_COUNT webpack compilations (expected 1)."
echo " This may indicate rake tasks are running multiple times."
FAILURES_FOUND=1
fi2. Minor: Possible Path Inconsistency (Line 116)Lines 116, 118, 132, and 225 use Verification needed: Check if 3. Enhancement: Add Exit Code Check (After Line 139)The current script only checks patterns in the output, but doesn't verify that Recommended addition: # Run precompile and capture exit code
RAILS_ENV=production bin/rake assets:precompile 2>&1 | tee precompile_output.txt
PRECOMPILE_EXIT_CODE=${PIPESTATUS[0]}
if [ $PRECOMPILE_EXIT_CODE -ne 0 ]; then
echo "::error::FAILURE: rake assets:precompile exited with status $PRECOMPILE_EXIT_CODE"
exit $PRECOMPILE_EXIT_CODE
fi4. Minor: Overly Broad Ruby Error Detection (Lines 183-186)Current: if grep -Ei "NameError|LoadError|NoMethodError|SyntaxError" precompile_output.txt; thenIssue: This could trigger false positives from comments, logs, or handled exceptions. Recommended refinement: # Look for actual error traces, not just the word "Error"
if grep -E "^[[:space:]]*(NameError|LoadError|NoMethodError|SyntaxError):" precompile_output.txt; then
echo "::error::FAILURE: Ruby errors detected during precompile."
FAILURES_FOUND=1
fiThis anchors the pattern to error message format, reducing false positives. 5. Enhancement: Show Matching Lines on FailureWhen a pattern matches, showing which lines matched would aid debugging: if grep -Ei "module not found|cannot find module|can't resolve" precompile_output.txt; then
echo "::error::FAILURE: Module resolution errors detected in precompile output."
echo "Matching lines:"
grep -Ei "module not found|cannot find module|can't resolve" precompile_output.txt | head -10
FAILURES_FOUND=1
fi🛡️ Security Considerations✅ No security concerns identified:
🧪 Test Coverage RecommendationsRecommended test plan:
📋 Adherence to CLAUDE.md Guidelines✅ Follows change detection pattern from existing workflows 🎯 Overall AssessmentQuality: High Required Changes:
Optional Enhancements:
Great work on this defensive CI addition! Once the duplicate detection logic is fixed, this will be a valuable safeguard against infrastructure regressions. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
.github/workflows/precompile-check.yml (4)
163-170: Add context to task duplication error for debugging.When
react_on_rails:generate_packsis detected running multiple times, the error message (line 167) only shows the count, not the actual lines or execution order. This makes it harder to understand what triggered the duplication.# Pattern 2: Duplicate task execution messages if grep -q "react_on_rails:generate_packs" precompile_output.txt; then GENERATE_PACKS_COUNT=$(grep -c "react_on_rails:generate_packs" precompile_output.txt || true) if [ "$GENERATE_PACKS_COUNT" -gt 1 ]; then echo "::error::FAILURE: react_on_rails:generate_packs task ran $GENERATE_PACKS_COUNT times (should only run once)." + echo " Matching lines:" + grep -n "react_on_rails:generate_packs" precompile_output.txt FAILURES_FOUND=1 fi fi
172-176: Add context to module resolution error for debugging.When module resolution errors are detected, only a generic message is shown. Including sample matching lines makes it easier to triage issues in CI.
# Pattern 3: Module not found errors if grep -Ei "module not found|cannot find module|can't resolve" precompile_output.txt; then echo "::error::FAILURE: Module resolution errors detected in precompile output." + echo " Sample matching lines:" + grep -Ei "module not found|cannot find module|can't resolve" precompile_output.txt | head -3 FAILURES_FOUND=1 fi
190-194: Sprockets error pattern could be case-sensitive mismatch.The pattern
"sprockets::filenotfound"(line 191) uses lowercase, but actual Ruby exception class names typically follow PascalCase (e.g.,Sprockets::FileNotFound). While-Eimakes the grep case-insensitive, the class name would be more clearly matched with the proper casing shown in the pattern.- if grep -Ei "sprockets::filenotfound|asset .* was not declared" precompile_output.txt; then + if grep -Ei "Sprockets::FileNotFound|Asset.*was not declared" precompile_output.txt; then echo "::error::FAILURE: Asset pipeline errors detected." + echo " Sample matching lines:" + grep -Ei "Sprockets::FileNotFound|Asset.*was not declared" precompile_output.txt | head -3 FAILURES_FOUND=1 fi
202-206: Warning count detection uses case-insensitive match; clarify intent or adjust threshold.The pattern
grep -ci "warning"matches any line containing "warning" (case-insensitive). This could include false positives from messages like "This is a warning to users" or unrelated logging. The threshold of >10 warnings is also arbitrary and may not reflect actual build health.Consider narrowing the pattern to specific warning types (e.g., webpack or Rails deprecation warnings) or document the current threshold choice:
# Pattern 8: Check for warnings that might indicate problems - WARNING_COUNT=$(grep -ci "warning" precompile_output.txt || true) + # Count webpack/build warnings (case-insensitive to catch "Warning:" and similar) + WARNING_COUNT=$(grep -Ei "warning|warn" precompile_output.txt | grep -v "User warning" | wc -l || true) if [ "$WARNING_COUNT" -gt 10 ]; then - echo "::warning::High number of warnings detected: $WARNING_COUNT warnings found. Please review." + echo "::warning::High number of build warnings detected: $WARNING_COUNT warnings found. Please review." + echo " Sample warnings:" + grep -Ei "warning|warn" precompile_output.txt | grep -v "User warning" | head -3 fiThis helps distinguish build warnings from other logged text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/precompile-check.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to lib/react_on_rails/*.rb : Create RBS signature files for new Ruby files in `lib/react_on_rails/` by adding corresponding `.rbs` file in `sig/react_on_rails/`, adding to Steepfile, and validating before committing
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration and must be linted separately
⏰ 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). (9)
- GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: build
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: precompile-check
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (1)
.github/workflows/precompile-check.yml (1)
222-228: Artifact configuration is sound for CI diagnostics.The artifact upload uses
if: always()to ensure logs are captured even on failure, includes a run-specific ID for uniqueness, and sets a reasonable 7-day retention window. This is good practice for debugging precompile failures.
Code Review: Assets Precompile Check WorkflowI've reviewed PR #2134 and overall this is a well-implemented and valuable addition to the CI pipeline. The workflow effectively addresses the issue of detecting silent failures during asset precompilation. Below are my findings: ✅ Strengths1. Excellent Pattern Detection StrategyThe workflow checks for 8 critical failure patterns that have caused production issues:
This comprehensive approach will catch both obvious and subtle failures. 2. Proper CI Integration
3. Good Error Handling
4. Helpful Debugging Features
🔍 Areas for Improvement1. Potential False Positives in Pattern DetectionIssue: Some patterns might be too broad and trigger false positives. Pattern 3 (lines 185-190): Module errorsgrep -Ei "module not found|cannot find module|can't resolve"Concern: This could match warnings in addition to actual errors. Webpack often emits deprecation warnings that contain these phrases but don't fail the build. Suggestion: Consider adding context to ensure these are actual errors: if grep -Ei "(ERROR|error).*module not found|(ERROR|error).*cannot find module|(ERROR|error).*can't resolve" precompile_output.txt; thenPattern 4 (lines 193-198): Webpack errorsgrep -Ei "webpack.*error|failed to compile|compilation failed|ERROR in"Concern: Suggestion: Use more specific patterns: grep -Ei "webpack.*ERROR|ERROR in [./]|compilation failed|failed to compile" precompile_output.txt2. Warning Threshold May Need TuningLine 226: Question: Is 10 warnings an appropriate threshold for this codebase? Suggestion:
3. Missing Pattern: Build Timeout/Hung ProcessesObservation: The workflow doesn't have a timeout specified for the precompile step. Risk: If webpack hangs, the job could run for the default 6-hour timeout. Suggestion: Add a timeout to the precompile step: - name: Run assets:precompile and check output
timeout-minutes: 20 # or whatever is reasonable
run: |4. Pattern Validation: Empty MatchesLines 163-230: Several patterns use Potential Issue: If Observation: Line 144 uses 5. Documentation: What Each Pattern PreventsSuggestion: Add comments above each pattern explaining which past incident it prevents: # Pattern 1: Prevents duplicate webpack compilations (regression test for PR #2052)
# Context: Rails::Engine was loading rake tasks twice, causing duplicate builds
if grep -q "Compiled successfully" precompile_output.txt; then
...
fiThis helps future maintainers understand why each check exists. 🔒 Security Considerations✅ No security concerns identified:
🎯 Performance ConsiderationsGood:
Minor:
🧪 Test CoverageFrom PR description test plan:
Suggestion: After merge, consider creating a follow-up issue to:
This validates the detection logic comprehensively. 📋 Checklist for MergeBefore merging, verify:
🎓 Alignment with Project GuidelinesChecked against ✅ Follows CI/CD patterns: Uses standard References:
✨ Final VerdictAPPROVED with minor suggestions ✅ This PR effectively solves the problem stated in issue #2081. The workflow is well-structured, follows existing patterns, and will catch critical production issues. The suggestions above are non-blocking improvements that can be addressed in follow-up PRs or ignored if the current implementation proves robust in practice. Recommended next steps:
Great work! This will significantly improve build reliability. 🚀 Review performed by Claude Code following project standards from CLAUDE.md |
|
@claude review this PR. |
|
Claude finished @justin808's task —— View job Code Review for PR #2134I've completed a thorough review of this PR. Overall, this is an excellent addition that addresses a real pain point (issue #2081) and follows the project's defensive programming philosophy. The implementation is well-structured and comprehensive. ✅ Strengths1. Addresses Real Problems
2. Excellent Pattern Detection StrategyThe validation script (
Each pattern includes:
3. Proper CI IntegrationThe workflow file (
4. Excellent Documentation
5. Smart Error Handling
🔍 Observations & Suggestions1. Pattern Validation: "killed" May Be Too Broad (Line 251)script/validate-precompile-output:251 The pattern Consideration: This may be acceptable given that OOM issues are critical. However, if false positives occur in practice, consider tightening to: grep -Ei "javascript heap out of memory|process killed|SIGKILL|out of memory"2. Warning Threshold Default of 20 (Line 265)The script sets
Suggestion: Document this threshold choice in the PR description or a comment so future maintainers understand the baseline. Consider monitoring actual warning counts in the first few CI runs and adjusting if needed. 3. Path Reference:
|
Closes #2081 This adds a new GitHub Actions workflow that runs `RAILS_ENV=production bin/rake assets:precompile` on the dummy app and checks the output for known failure patterns: - Duplicate webpack compilations (tasks running twice) - Duplicate rake task execution (e.g., generate_packs running multiple times) - Module not found / cannot resolve errors - Webpack compilation errors - Ruby errors (NameError, LoadError, etc.) - Asset pipeline errors - Memory issues (heap out of memory) - High warning counts (>10 warnings triggers a warning annotation) The workflow runs on PRs and master pushes, uses the same change detection as other CI jobs, and uploads the precompile output as an artifact for debugging when failures occur. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The precompile check needs to run react_on_rails:generate_packs first to generate the server-bundle-generated.js file that webpack imports. This matches what other CI jobs do before running webpack builds. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The dummy app uses ReScript components that need to be compiled before webpack can bundle them. This adds `pnpm run build:rescript` to compile HelloWorldReScript.res.js and other ReScript files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Changes based on CodeRabbit review comments: 1. Capture rake exit code with PIPESTATUS to detect silent failures - Added set -o pipefail to propagate pipe failures - Check PIPESTATUS[0] after tee to catch rake failures 2. Improved duplicate webpack compilation detection - Changed pattern from generic "Compiled" to "Compiled successfully" - Simplified logic to count successful compilations directly - Added line numbers to matching output for debugging 3. Improved webpack error pattern detection - Changed from overly broad "error in" to more specific patterns - Now matches: webpack.*error, failed to compile, compilation failed, ERROR in 4. Better Ruby error detection - Pattern now matches error class format with colon (e.g., "NameError:") - Reduces false positives from log messages 5. Fixed asset pipeline error pattern - Use proper casing for Sprockets::FileNotFound 6. Added sample output for all error patterns - Each error now shows the first few matching lines for easier debugging 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added checks for: - Duplicate locale generation (react_on_rails:locale task) - Duplicate webpack builds (via "Built at:" message count) - ENOENT errors (missing files/directories) - Deprecation warnings (logged as warning, not failure) These patterns were explicitly requested in the updated issue description to catch silent bugs that don't cause precompile to fail but indicate problems like doubled build times or missing dependencies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Extracted all pattern detection logic into script/validate-precompile-output so it can be: - Run locally: script/validate-precompile-output <output_file> - Piped: RAILS_ENV=production rake assets:precompile 2>&1 | script/validate-precompile-output - - Used by CI with GitHub Actions annotations The script supports both local and CI usage, automatically using GitHub Actions annotation format when GITHUB_ACTIONS env var is set. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Detect empty output (0 bytes) as failure, indicating command failed before producing any output - Add warning for suspiciously small output (<10 lines by default) - Add Pattern 5b: Detect bundler/gem errors like "Could not find gem" - Fix shellcheck warning: use single quotes in trap command - Update usage docs to emphasize pipefail requirement when piping - Add timeout-minutes: 15 comment to CI workflow step The empty output detection addresses the case where rake fails immediately (e.g., missing gem) but produces no stdout, causing the validation to incorrectly pass when piped without pipefail. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Show context (20 lines) after COMPILATION FAILED to reveal actual error - Add specific pattern to detect missing webpack-cli with actionable fix - The validation script now provides much clearer error messages 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The dummy app's JS dependencies were missing a lockfile, which meant dependency versions could vary between installs. This lockfile ensures reproducible builds for local development and CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The pnpm-lock.yaml in spec/dummy uses link: protocol for the react-on-rails dependency, but yalc add (without --link) changes package.json to use file: protocol, causing ERR_PNPM_OUTDATED_LOCKFILE errors in CI. Adding --link flag ensures yalc maintains the link: protocol that matches the lockfile, fixing all CI workflow failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Ensures CI tests a fresh webpack build rather than potentially getting "Everything's up-to-date" from cached assets. The clobber task removes both Sprockets assets (public/assets) and webpack output (public/webpack). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The minimum config job switches shakapacker from 9.4.0 to 8.2.0 via bin/ci-switch-config, which modifies package.json. This causes pnpm-lock.yaml to be out of sync, requiring --no-frozen-lockfile. Added conditional --no-frozen-lockfile flag to dummy app pnpm install steps when running with minimum dependency level. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
jg/precompile-ci-check
branch
from
8795487 to
24c93f0
Compare
Code Review: Assets Precompile Check CI WorkflowExcellent work on this PR! This adds valuable infrastructure to catch build issues early. Here's my detailed review: ✅ Strengths1. Comprehensive Error DetectionThe
The pattern detection is well-thought-out with good historical context in comments. 2. Well-Documented Script
3. GitHub Actions Integration
4. Production RealismRunning
|
Changes based on review suggestions: 1. Remove pnpm-lock.yaml from dummy app - The dummy app didn't have a lockfile before this PR - Keeping it could cause issues for local development with yalc - Updated workflow comment to clarify no lockfile exists 2. Fix memory error pattern to be more specific - Changed "killed" to "\bKilled\b" with word boundary - Use capital K which matches OOM killer message format - Prevents false positives from "killed the bug" etc. 3. Update module error comment for accuracy - Clarified that primary match is webpack's "Module not found: Error:" format - Also matches ERROR/Error: prefixed messages 4. Document duplicate detection rationale - Explained why both "Compiled successfully" and "Built at:" are checked - "Compiled successfully" may be absent on failed builds - "Built at:" is always present, catches duplicates on any outcome 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
script/validate-precompile-output (1)
122-141: Consider limiting output for duplicate task detection.Patterns 2a and 2b don't limit their
grep -noutput like other patterns do. If there are many duplicates, this could produce verbose output.if [ "$GENERATE_PACKS_COUNT" -gt 1 ]; then report_error "react_on_rails:generate_packs task ran $GENERATE_PACKS_COUNT times (should only run once)." echo " Matching lines:" - grep -n "react_on_rails:generate_packs" "$OUTPUT_FILE" + grep -n "react_on_rails:generate_packs" "$OUTPUT_FILE" | head -5 FAILURES_FOUND=1 fi fi # Pattern 2b: Duplicate locale generation (same root cause as 2a) if grep -q "react_on_rails:locale" "$OUTPUT_FILE"; then LOCALE_COUNT=$(grep -c "react_on_rails:locale" "$OUTPUT_FILE" || true) if [ "$LOCALE_COUNT" -gt 1 ]; then report_error "react_on_rails:locale task ran $LOCALE_COUNT times (should only run once)." echo " Matching lines:" - grep -n "react_on_rails:locale" "$OUTPUT_FILE" + grep -n "react_on_rails:locale" "$OUTPUT_FILE" | head -5 FAILURES_FOUND=1 fi fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/integration-tests.yml(2 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/precompile-check.yml(1 hunks)script/validate-precompile-output(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/precompile-check.yml
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS ensure files end with a newline character before committing/pushing
Files:
script/validate-precompile-output
🧠 Learnings (5)
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Applied to files:
.github/workflows/lint-js-and-ruby.yml
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration and must be linted separately
Applied to files:
.github/workflows/lint-js-and-ruby.yml.github/workflows/integration-tests.yml
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
.github/workflows/lint-js-and-ruby.yml
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
.github/workflows/integration-tests.yml
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/**/*.spec.js : Use Rails helpers (`appFactories`, `appEval`) in Playwright tests instead of UI-based setup to leverage factory_bot and arbitrary Ruby code execution
Applied to files:
.github/workflows/integration-tests.yml
⏰ 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). (10)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (15)
.github/workflows/lint-js-and-ruby.yml (1)
135-136: LGTM!The
--linkflag is the correct approach to maintain consistency with pnpm'slink:protocol in the lockfile. The comment clearly explains the rationale..github/workflows/integration-tests.yml (4)
153-154: LGTM!Consistent use of
--linkflag matching the lint workflow. The comment explains the rationale.
157-158: LGTM!The conditional lockfile handling correctly accounts for the minimum dependency configuration's package.json modifications. This is consistent with line 148's approach.
260-261: LGTM!Consistent with the changes in the build job.
264-265: LGTM!Consistent with the build job's approach at line 158.
script/validate-precompile-output (10)
1-45: Well-structured script with good documentation.The header documentation, input handling, and temp file cleanup are well implemented. The
trapensures proper cleanup on exit.
47-70: LGTM!Good validation of output integrity. The distinction between hard failure (empty file) and soft warning (low line count) is appropriate. The configurable threshold provides flexibility.
75-95: LGTM!Clean helper functions with proper GitHub Actions annotation support. The
$GITHUB_ACTIONSenvironment variable check is the standard way to detect CI context.
97-114: LGTM!Good detection logic with helpful context. The
|| truecorrectly preventsset -efrom triggering on zero matches withgrep -c.
164-169: LGTM!The regex carefully balances catching real errors while avoiding false positives from informational messages. The documentation explains the matching strategy well.
179-215: LGTM!Good defensive pattern matching with the
^ERROR inanchor to avoid false positives. The context extraction aroundCOMPILATION FAILEDand actionable webpack-cli fix instructions are helpful.
217-238: LGTM!Matching specific exception class names is a good approach to reduce false positives. The Bundler error detection covers common scenarios with actionable guidance.
251-262: LGTM!The
\bKilled\bword boundary correctly avoids false positives while catching the OOM killer's output format.
264-289: LGTM!Good advisory patterns that surface potential issues without blocking CI. The configurable threshold and clear distinction between fatal errors and warnings is well designed.
291-304: LGTM!Clear summary output with appropriate exit codes matching the documented behavior. The file correctly ends with a newline as per coding guidelines.
Code Review - PR #2134: Add CI workflow to check assets:precompile outputExcellent work on this proactive quality check! This is exactly the kind of defensive measure that prevents silent regressions. ✅ Strengths1. Well-Designed Workflow
2. Comprehensive Pattern DetectionThe validation script covers a wide range of issues including duplicate task execution, module resolution errors, webpack compilation failures, Ruby exceptions, bundler/gem issues, memory issues, and asset pipeline errors. 3. Excellent Documentation
4. Good CI Practices
🔍 Issues & SuggestionsCritical: Verify Executable FlagEnsure the script executable bit is tracked in git. Run: git ls-files -s script/validate-precompile-output If it shows 100644, add permission: git update-index --chmod=+x script/validate-precompile-output Pattern Detection RefinementsLine 164: Overly Broad Module Error Pattern - Current pattern could match informational text. Suggest making it more specific to actual error contexts. Line 190: Webpack Error Pattern - The pattern "failed to compile" could match documentation text. Suggest anchoring to webpack-specific output. Line 232: Bundler Pattern - The pattern "Run bundle install" could appear in documentation. Suggest only matching actual error output. Workflow ConfigurationCurrently runs only with Ruby 3.4 and Node 22. Consider adding matrix strategy later if version-specific issues are discovered. 📝 Additional SuggestionsAdd Test SuiteCreate script/test-validate-precompile-output with test cases to ensure patterns work correctly. Document Expected OutputAdd comments showing typical successful output to help future maintainers. 🎯 Performance ConsiderationsImpact: Adds ~2-5 minutes to CI. The workflow properly reuses pnpm cache, gem cache, and runs in parallel with other jobs. ✅ Testing Requirements (Per CLAUDE.md)Before merging, please verify locally:
📚 DocumentationSuggest adding to CLAUDE.md: Precompile Output ValidationThe precompile-check workflow validates assets:precompile output for known failure patterns. Run locally with: RAILS_ENV=production rake assets:precompile 2>&1 | script/validate-precompile-output - SummaryOverall Assessment: ⭐⭐⭐⭐⭐ (5/5) This is an excellent defensive measure that will catch regressions early. The implementation is solid, well-documented, and follows best practices. Recommendation: Approve with minor refinements Before Merging:
Future Enhancements (not blocking):
Great work! This will save the team significant debugging time in the future. 🤖 Generated with Claude Code |
Summary
Closes #2081
This adds a new GitHub Actions workflow that runs
RAILS_ENV=production bin/rake assets:precompileon the dummy app and checks the output for known failure patterns that have been encountered in the past.Failure Patterns Detected
The workflow checks for:
generate_packsrunning multiple timesFeatures
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests / CI
✏️ Tip: You can customize this high-level summary in your review settings.