Enhance change detection & workflow triggers #2020
Conversation
Code Review SummaryPR Type: Test PR (Empty Commit) OverviewThis PR contains an empty commit created to reproduce CI results from a previous commit. There are no code changes to review. Review Findings✅ Code Quality: N/A - No code changes CI StatusAll CI checks are passing or appropriately skipped:
Observations
RecommendationThis is a valid test PR for CI debugging purposes. No code review feedback applicable. Close without merging once the CI investigation is complete. Review conducted by Claude Code following repository conventions from CLAUDE.md |
|
Warning Rate limit exceeded@Judahmeek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 33 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 (1)
WalkthroughMultiple CI workflows and the change-detection script were updated: path-based trigger ignores added, a Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Event
participant Detector as script/ci-changes-detector
participant WorkflowCond as Workflow Conditions
participant Runner as GitHub Actions Runner
GH->>Detector: event + changed paths + inputs (force_run, labels)
Note over Detector: Classify changes into core and Pro categories\nSet flags (RUBY/JS/RSPEC/PRO_*)
alt force_run = true OR full-ci label present
Detector->>WorkflowCond: set run_all = true
else
Detector->>WorkflowCond: emit granular flags for gating
end
WorkflowCond->>Runner: enable/disable jobs (lint, tests, pro-tests, dummy)
Runner->>Runner: execute enabled jobs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes -重点审查项:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Review: Test PR with empty commitOverviewThis PR updates GitHub Actions workflow configurations to add path filtering and workflow dispatch options. The changes aim to optimize CI runs by skipping workflows when irrelevant files change. Code Quality & Best Practices✅ Strengths
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/workflows/lint-js-and-ruby.yml (1)
16-22: Unusedforce_runinput — recommend implementing bypass logic.The
force_runinput is declared but not used in thedetect-changesstep. Compare with.github/workflows/integration-tests.ymlline 48, which properly checksif [ "${{ inputs.force_run }}" = "true" ]. Add this logic to make the input functional.id: detect run: | - # If full-ci label is present, run everything - if [ "${{ steps.check-label.outputs.result }}" = "true" ]; then + # If force_run is true OR full-ci label is present, run everything + if [ "${{ inputs.force_run }}" = "true" ] || [ "${{ steps.check-label.outputs.result }}" = "true" ]; then echo "run_lint=true" >> "$GITHUB_OUTPUT".github/workflows/package-js-tests.yml (1)
20-26: Unusedforce_runinput — recommend implementing bypass logic.The
force_runinput is declared but not checked in thedetect-changesstep (lines 48–64). For consistency with other workflows and to make this input functional, add the force_run check:if [ "${{ inputs.force_run }}" = "true" ] || [ "${{ steps.check-label.outputs.result }}" = "true" ].id: detect run: | - # If full-ci label is present, run everything - if [ "${{ steps.check-label.outputs.result }}" = "true" ]; then + # If force_run is true OR full-ci label is present, run everything + if [ "${{ inputs.force_run }}" = "true" ] || [ "${{ steps.check-label.outputs.result }}" = "true" ]; then echo "run_lint=true" >> "$GITHUB_OUTPUT".github/workflows/examples.yml (1)
1-1: TODO comment in workflow name — consider tracking as an issue.The appended TODO comment about duplicating this workflow for RoR Pro is useful context, but workflow names should remain clean for clarity. Consider creating a GitHub issue to track this work instead of embedding it in the workflow name.
-name: Generator tests # TODO needs to be duplicated for RoR Pro +name: Generator testsWith a corresponding GitHub issue created to track the duplication work for Pro.
.github/workflows/playwright.yml (1)
9-15: Note:force_runinput declared but has no effect in this workflow.The
force_runinput is declared but this workflow lacks adetect-changesjob or conditional logic that would consume it. Clarify intent: if this workflow should always run onworkflow_dispatch, the input may be unnecessary. If you'd like to add smart change-detection (e.g., skip on docs-only changes), implement adetect-changesjob and conditionalifstatements on jobs, similar to other workflows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/examples.yml(1 hunks).github/workflows/integration-tests.yml(2 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/package-js-tests.yml(1 hunks).github/workflows/playwright.yml(1 hunks).github/workflows/pro-gem-tests.yml(1 hunks).github/workflows/pro-integration-tests.yml(1 hunks).github/workflows/pro-lint.yml(1 hunks).github/workflows/rspec-gem-specs.yml(1 hunks)script/ci-changes-detector(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
.github/workflows/integration-tests.yml.github/workflows/package-js-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/integration-tests.yml
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
.github/workflows/package-js-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). (2)
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (11)
.github/workflows/integration-tests.yml (3)
1-1: Good workflow rename for clarity.Renaming from "Main test" to "Integration Tests" better describes the workflow's scope and aligns with the PR's objective to organize CI workflows.
7-15: Appropriate path-ignore patterns for change detection.The addition of
react_on_rails_pro/**to both push and pull_request path-ignore entries aligns with the broader CI strategy to skip this workflow when only Pro-specific changes occur.
16-22: Well-implemented force_run input.The
force_runinput is properly declared with sensible defaults (boolean, default false), and the detect-changes step correctly implements the logic to bypass change detection when triggered..github/workflows/pro-lint.yml (2)
7-19: Consistent path-ignore configuration with Pro-specific exclusions.The path-ignore patterns exclude documentation, non-Pro packages, and test files, ensuring the Pro lint workflow only runs on meaningful changes to the Pro codebase.
21-26: Properly implemented force_run input with correct detect-changes logic.The workflow correctly implements the force_run bypass pattern, allowing developers to override change detection when needed for debugging or manual CI runs.
.github/workflows/pro-gem-tests.yml (2)
7-12: Appropriate path-ignore patterns for Pro package tests.Path-ignore entries exclude documentation, core library code, and main package code, ensuring the Pro tests workflow focuses only on Pro-specific changes.
Also applies to: 14-19
21-26: Force_run input properly integrated with change detection logic.The workflow correctly implements the force_run input with proper detect-changes logic at line 53.
.github/workflows/pro-integration-tests.yml (1)
7-12: Consistent path-ignore patterns for Pro integration tests.Path-ignore entries exclude documentation, core code, and non-Pro packages, aligning with the broader CI strategy to minimize unnecessary workflow runs on non-code changes.
Also applies to: 14-19
.github/workflows/examples.yml (1)
14-20: Force_run input properly implemented with bypass logic.The workflow correctly handles the force_run input in the detect-changes step (line 46), ensuring developers can manually trigger full CI runs when needed.
.github/workflows/playwright.yml (1)
6-8: Added path-ignore patterns improve workflow efficiency.The path-ignore entries for Markdown and docs ensure the Playwright E2E workflow skips unnecessary runs on documentation-only changes to master branch.
.github/workflows/rspec-gem-specs.yml (1)
10-11: LGTM! Path-ignore patterns are more maintainable.Consolidating the ignore patterns from specific subdirectories (
packages/react-on-rails/src/**,node_package/src/**) to broader top-level directories (packages/**) and addingreact_on_rails_pro/**makes the configuration simpler and more maintainable.Also applies to: 16-17
Code ReviewThanks for working on optimizing the CI workflows! This PR makes sensible improvements to reduce unnecessary CI runs. Here's my detailed feedback: ✅ Strengths
|
Code ReviewThank you for this PR! This is a nice improvement to the CI optimization strategy. I've reviewed the changes and have the following feedback: ✅ Strengths
🐛 Potential Issues1. Inconsistent Pro Package Filtering (High Priority)The OSS workflows correctly exclude react_on_rails_pro/, but the Pro workflows have overly broad exclusions that exclude ALL of lib/, spec/, and packages/. Problem: Changes to core React on Rails code can affect the Pro package. Changes to lib/react_on_rails/helper.rb should trigger Pro tests, and changes to packages/react-on-rails/src/ could break Pro features since Pro depends on the OSS gem as a foundation. Recommendation: Either remove these paths-ignore from Pro workflows entirely (safest), or use more specific exclusions. 2. Missing force_run Implementation in Some WorkflowsWhile force_run input is added to several workflows, the detect-changes job doesn't always check it. The input is defined but never checked in lint-js-and-ruby.yml, package-js-tests.yml, and rspec-gem-specs.yml. Fix: Add force_run check to all workflows that define it (see pro-lint.yml for reference implementation). 3. Playwright Workflow Doesn't Need Change Detectionplaywright.yml adds paths-ignore and force_run input, but has no detect-changes job. This means the force_run input does nothing. Recommendation: Either add change detection (like other workflows) or remove the unused force_run input. 4. TODO Comment Needs ActionThe TODO comment about duplicating Generator tests for RoR Pro should either be addressed in this PR or tracked in an issue. 📊 Testing RecommendationsBefore merging, please verify:
Overall AssessmentThis is a solid foundation for CI optimization, but the Pro package path filtering needs attention before merge. The current exclusions could cause Pro CI to skip when it should run, potentially breaking Pro features. Recommended Action:
Review generated by Claude Code following project conventions from CLAUDE.md |
|
I have reviewed PR #2020. Here are my findings: Strengths:
Critical Issues:
Testing Recommendations:
Overall: Good optimization work but needs completion of workflow_dispatch logic and verification of git renames. |
Judahmeek
changed the title
PR Review: Test PR with empty commitOverviewThis PR implements CI optimization improvements by adding path-based filtering to GitHub Actions workflows and extracting actionlint into a separate workflow. Critical Issues Found1. JSON Syntax Error (script/ci-changes-detector:253) 2. Incorrect Workflow Name (actionlint.yml:1) 3. Push Event Handling (actionlint.yml:36) Strengths
RecommendationsBefore merging:
The architectural improvements are sound, but please address the critical issues first. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
script/ci-changes-detector (1)
89-89: Verify generator file patterns match actual filenames.A previous review noted that the generator patterns may have filename mismatches. Specifically:
rakelib/example_config.ymlmay need to berakelib/examples_config.yml(plural)rakelib/examples.rakemay not existRun this script to verify the actual filenames:
#!/bin/bash echo "=== Checking generator-related rakelib files ===" fd -t f . rakelib/ | grep -E "(example|generator)" echo "" echo "=== Files referenced in pattern (line 89) ===" echo "- rakelib/example_type.rb" echo "- rakelib/example_config.yml" echo "- rakelib/shakapacker_examples.rake".github/workflows/gem-tests.yml (1)
18-24: Theforce_runinput is still not checked in the detect-changes job.The
force_runinput was added but is never referenced in the workflow. Looking at lines 49-58, the detect-changes step only checks for thefull-cilabel but doesn't honorinputs.force_run.Apply this diff to check the
force_runinput:- name: Detect relevant changes id: detect run: | + # If force_run is true OR full-ci label is present, run everything + if [ "${{ inputs.force_run }}" = "true" ] || [ "${{ steps.check-label.outputs.result }}" = "true" ]; then - # If full-ci label is present, run everything - if [ "${{ steps.check-label.outputs.result }}" = "true" ]; then
🧹 Nitpick comments (2)
.github/workflows/actionlint.yml (2)
1-1: Workflow name is misleading.The workflow is named "Lint JS and Ruby" but only runs actionlint for GitHub Actions workflows. Consider renaming to "Lint GitHub Actions Workflows" or "Actionlint" to accurately reflect its purpose.
Apply this diff:
-name: Lint JS and Ruby +name: Lint GitHub Actions Workflows
14-18: Theforce_runinput is declared but never used.The workflow declares a
force_runinput but never checks it. Since the workflow already gates execution on workflow file changes (line 37), theforce_runinput could be useful to bypass that check when needed.Consider either:
- Using the input to skip the change detection (lines 33-48) when
force_runis true, or- Removing the unused input if it's not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/actionlint.yml(1 hunks).github/workflows/gem-tests.yml(1 hunks).github/workflows/lint-js-and-ruby.yml(2 hunks).github/workflows/main.yml(2 hunks).github/workflows/pro-integration-tests.yml(4 hunks).github/workflows/pro-lint.yml(2 hunks).github/workflows/pro-test-package-and-gem.yml(3 hunks)script/ci-changes-detector(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/lint-js-and-ruby.yml
- .github/workflows/pro-lint.yml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
script/ci-changes-detector
📚 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:
script/ci-changes-detector.github/workflows/main.yml
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
script/ci-changes-detector
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
.github/workflows/main.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). (3)
- GitHub Check: lint-js-and-ruby
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (7)
.github/workflows/main.yml (1)
10-10: LGTM! Path-ignore additions align with Pro exclusion strategy.Adding
react_on_rails_pro/**to the path-ignore list is consistent with the broader CI optimization effort to separate React on Rails Pro workflows from the main integration tests.Also applies to: 15-15
.github/workflows/gem-tests.yml (1)
10-11: LGTM! Path-ignore broadening is appropriate.Expanding from specific source paths to
packages/**and addingreact_on_rails_pro/**simplifies the exclusion rules while maintaining the same intent of skipping gem tests when only package or Pro code changes.Also applies to: 16-17
.github/workflows/pro-test-package-and-gem.yml (2)
86-86: Switching to yarn cache is a good improvement.Enabling yarn caching will speed up these jobs now that the Node 22 V8 bug workaround is no longer needed.
Also applies to: 178-178
7-19: Path-ignore exclusions create a process gap—root lib changes won't trigger Pro tests despite Pro's dependency on the root gem.The verification reveals a substantive concern. While the path-ignore patterns (
lib/**,spec/**) are processed at the workflow level, theci-changes-detectorscript enforces a deliberate separation: it only setsrun_pro_tests=truewhen Pro-specific code changes (PRO_RUBY_CHANGED,PRO_RSPEC_CHANGED,PRO_JS_CHANGED), never when rootlib/**changes (RUBY_CHANGED).This creates a process gap: if a developer modifies only root
lib/**(without touching Pro code), the entire workflow skips due to path-ignore patterns. More critically, the ci-changes-detector script contains no logic to trigger Pro tests when rootlib/**changes—even if the workflow did run. Since Pro's gemspec explicitly declares a runtime dependency on the rootreact_on_railsgem, changes to rootlib/**can affect Pro functionality but won't execute Pro tests.Verify whether this design is intentional (i.e., Pro tests are isolated unit tests with integration tested elsewhere) or a missed edge case in change detection logic. If intentional, document this assumption. If not, consider adding a rule to
ci-changes-detectorthat setsRUN_PRO_TESTS=truewhen rootRUBY_CHANGED=true..github/workflows/pro-integration-tests.yml (2)
7-19: Verify that path-ignore exclusions won't skip necessary integration tests.Similar to pro-test-package-and-gem.yml, this workflow excludes
lib/**,spec/**, andpackages/react_on_rails/**. The exclusion ofpackages/react_on_rails/**makes sense since Pro has its own package.However, ensure that changes to root
lib/**truly never affect Pro integration tests, especially if Pro depends on shared utilities or the base gem.The same verification script from pro-test-package-and-gem.yml applies here—check for cross-references between root and Pro code.
86-86: Enabling yarn cache across all Node setup steps is a solid optimization.Consistent caching will improve CI performance for all three jobs that use Node.
Also applies to: 173-173, 361-361
script/ci-changes-detector (1)
57-67: Pro-specific change tracking is well-structured.The addition of granular Pro flags (
PRO_LINT_CONFIG_CHANGED,PRO_RSPEC_CHANGED,PRO_JS_CHANGED,PRO_DUMMY_CHANGED) provides fine-grained control over which Pro CI jobs run, mirroring the structure used for the main React on Rails codebase. The logic correctly propagates these flags through the decision tree.Also applies to: 107-140, 184-225
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Code Review - PR #2020: Enhance change detection & workflow triggersOverviewThis PR makes significant improvements to CI/CD workflow efficiency by:
🔍 Code Quality & Best Practices✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/actionlint.yml (1)
50-58: Potential issue: incomplete error handling for API call failures.If the curl request at line 50 fails (e.g., network timeout, rate limiting),
actionlint_versionis never populated inGITHUB_OUTPUT. This results in an incomplete cache key at line 66 (ubuntu-22.04-actionlint-with empty version). While the workflow still functions (the cache miss is handled at line 68), this leads to cache key pollution and prevents effective caching across runs.Add explicit error handling and fallback logic:
response=$(curl -sf https://api.github.com/repos/rhysd/actionlint/releases/latest) if [ $? -eq 0 ]; then actionlint_version=$(echo "$response" | jq -r .tag_name) if [ -z "$actionlint_version" ]; then echo "Failed to parse Actionlint version" exit 1 fi echo "actionlint_version=\"$actionlint_version\"" >> "$GITHUB_OUTPUT" + else + echo "Warning: Failed to fetch Actionlint version from API; using fallback cache key" + echo "actionlint_version=\"latest\"" >> "$GITHUB_OUTPUT" fiThis ensures the cache key is always populated consistently, even if the API call fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/actionlint.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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). (3)
- GitHub Check: lint-js-and-ruby
- GitHub Check: build
- GitHub Check: claude-review
Code Review - PR #2020: Enhance change detection & workflow triggersSummaryThis PR enhances CI efficiency by improving change detection logic and adding path-based workflow triggers. The changes significantly reduce unnecessary CI runs while maintaining comprehensive test coverage. ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
script/ci-changes-detector (1)
88-92: Fix generator config filename in pattern (still using singular form)
casepattern at Line 89 still matchesrakelib/example_config.yml, but prior analysis showed the actual file israkelib/examples_config.yml(plural). That means changes to the real config file won’t setGENERATORS_CHANGED=true, so generator-related CI won’t run as intended.Suggested fix:
- lib/generators/*|lib/generators/**/*|rakelib/example_type.rb|rakelib/example_config.yml|rakelib/shakapacker_examples.rake|.github/workflows/examples.yml) + lib/generators/*|lib/generators/**/*|rakelib/example_type.rb|rakelib/examples_config.yml|rakelib/shakapacker_examples.rake|.github/workflows/examples.yml)You can confirm the filename with a quick scan:
#!/bin/bash fd 'examples?_config\.yml' rakelib -a
🧹 Nitpick comments (2)
.github/workflows/lint-js-and-ruby.yml (1)
82-82: Double-check enabling Yarn cache for Node 22 vs integration-tests.ymlHere the lint job uses Node 22 with
cache: yarn, butintegration-tests.ymlexplicitly disables Yarn caching for Node 22 due to the referenced V8 bug in 22.21.0. If that issue is still relevant for this project, you may want the same “no cache on Node 22” behavior here, or at least a short comment explaining why caching is considered safe for linting.Please confirm against the Node 22 / V8 issue you reference in
integration-tests.yml(actions/setup-node+ Yarn caching) that enabling the cache here won’t reintroduce flakiness, and adjust or document accordingly.script/ci-changes-detector (1)
57-66: Pro change flags and gating logic are coherent and match the OSS sideThe new
PRO_*flags (PRO_RUBY_CHANGED,PRO_JS_CHANGED,PRO_RSPEC_CHANGED,PRO_DUMMY_CHANGED,PRO_LINT_CONFIG_CHANGED) and their RUN_PRO_* conditions form a clear, parallel structure to the non‑Pro flags:
- Lint runs for any relevant code or lint-config changes (Pro or OSS).
- Pro gem tests run on Pro Ruby/spec/JS changes but not on dummy-only changes.
- Pro dummy integration runs on Pro dummy changes and on Pro Ruby/JS changes.
This keeps the Pro CI behavior explicit and avoids over-triggering on docs-only changes. The comment on
GENERATORS_CHANGEDabout duplicating to Pro is a fine future enhancement but not a blocker.Also applies to: 160-171, 176-185, 186-216, 218-225
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/actionlint.yml(1 hunks).github/workflows/gem-tests.yml(2 hunks).github/workflows/integration-tests.yml(2 hunks).github/workflows/lint-js-and-ruby.yml(3 hunks).github/workflows/package-js-tests.yml(2 hunks).github/workflows/playwright.yml(1 hunks).github/workflows/pro-test-package-and-gem.yml(3 hunks)script/ci-changes-detector(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/package-js-tests.yml
- .github/workflows/playwright.yml
- .github/workflows/actionlint.yml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
script/ci-changes-detector
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
.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/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). (5)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (7)
.github/workflows/pro-test-package-and-gem.yml (3)
7-12: Good: Path-ignore filters reduce unnecessary CI runs for non-Pro changes.The filters correctly exclude Markdown, documentation, main package, and test directories. This aligns with the workflow's Pro-specific focus and prevents CI overhead from commits that don't affect Pro package builds.
Also applies to: 14-19
86-87: Good: Node cache setup improvements unlock Yarn caching for Node 22.Replacing the previous explicit cache setting with
cache: yarnand propercache-dependency-pathglob is cleaner and resolves the prior V8-related blocker. The glob patternreact_on_rails_pro/**/yarn.lockcorrectly targets Pro package dependencies.Also applies to: 178-179
22-26: Verification complete: ci-changes-detector script correctly implements all required functionality.The script exists, is executable, and properly outputs all expected environment variables (
run_pro_lint,run_pro_tests,docs_only). Parameter handling correctly processes the BASE_REF argument with fallback toorigin/master, includes validation checks, and integrates properly with the git diff logic. The implementation supports the workflow's Pro-specific change detection as intended..github/workflows/integration-tests.yml (1)
1-15: Workflow rename and Pro path-ignore look consistentRenaming to “Integration Tests” and ignoring
react_on_rails_pro/**for push/PR cleanly scopes this workflow to the OSS dummy app and aligns with the separate Pro workflows. Nothing concerning here..github/workflows/gem-tests.yml (1)
10-24: force_run wiring and broader path filters match the intended CI behaviorAdding
packages/**andreact_on_rails_pro/**topaths-ignorekeeps this gem workflow focused on Ruby-only changes, while the newworkflow_dispatch.force_runinput is correctly honored in the detect step to bypass change detection and turn on all run_* flags / disabledocs_only.This now matches the “Force run all jobs (bypass detect-changes)” description and the full‑CI label semantics.
To be safe, trigger this workflow via Run workflow with
force_runchecked once and confirm both dependency levels run as expected on the branch you care about.Also applies to: 49-51
.github/workflows/lint-js-and-ruby.yml (1)
10-22: Lint workflow now follows the same force_run / Pro scoping patternIgnoring
react_on_rails_pro/**for push/PR and adding theworkflow_dispatch.force_runinput, then honoring it in the detect step to flip all run_* flags, keeps this workflow focused on OSS changes while still allowing a manual “run everything” escape hatch. This aligns nicely with the gem and package workflows.It’s worth doing a quick manual
workflow_dispatchrun withforce_runenabled to confirmdetect-changessetsrun_linttotrueand thebuildjob runs regardless of file changes.Also applies to: 46-47
script/ci-changes-detector (1)
165-171: Pro-related summaries and outputs are aligned and JSON is validThe updated “Changed file categories” and “Recommended CI jobs” sections now clearly call out the Pro categories and RUN_PRO_* decisions, and the new
run_pro_lint,run_pro_tests, andrun_pro_dummy_testsvalues are exported both via$GITHUB_OUTPUTand the CI_JSON_OUTPUT block (with the comma after"run_pro_tests"fixed).This keeps human-readable logs, workflow outputs, and any JSON consumers in sync.
Also applies to: 218-225, 237-239, 253-254
PR Review: Enhance change detection & workflow triggersOverviewThis PR improves CI efficiency by:
✅ Strengths
🔍 Issues & Concerns1. Critical: Missing paths-ignore in actionlint.yml# .github/workflows/actionlint.yml
on:
pull_request:
paths:
- '.github/workflows/**'Issue: Using Recommendation: Either:
Preferred approach: Keep 2. Bash Safety: Potential issues in actionlint.ymlThe bash script has some safety concerns:
3. Logic Issue: ci-changes-detector Pro path handlingShell glob patterns in case statements do not support Recommendation: Test this locally with actual file paths to ensure patterns match correctly. 4. Potential Race Condition: Caching actionlintIf the 5. Documentation: Missing workflow rename documentationSeveral workflows were renamed:
Recommendation: Update any documentation that references these workflow names, particularly in:
📊 Performance Considerations
Estimated CI Time Savings: 2-5 minutes per PR (depending on which workflows are skipped) 🔒 Security Considerations
🧪 Test CoverageIssue: This PR changes CI behavior but does not include tests for the Recommendation: Consider adding a test suite for 📝 Additional Recommendations
✅ Pre-merge Checklist
Overall AssessmentQuality: 7/10 The PR is well-intentioned and addresses real pain points. The main concerns are around the actionlint workflow having redundant logic (both The most critical item to address is clarifying the actionlint workflow strategy - either use the Review generated with context from CLAUDE.md project guidelines |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
script/ci-changes-detector (1)
101-101: Wrong workflow filename reference.Line 101 references
.github/workflows/integration-tests.yml, but according to previous review comments, the actual workflow file should be.github/workflows/main.yml.Apply this diff:
- spec/dummy/*|spec/dummy/**/*|.github/workflows/integration-tests.yml) + spec/dummy/*|spec/dummy/**/*|.github/workflows/main.yml)
🧹 Nitpick comments (1)
script/ci-changes-detector (1)
159-159: Address the TODO for node-renderer tests.The TODO comment indicates that node-renderer test detection is missing. If node-renderer tests exist and should trigger CI jobs, add the appropriate detection pattern and flag.
Do you want me to help identify where node-renderer tests are located and generate the detection logic for them?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/actionlint.yml(1 hunks)script/ci-changes-detector(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
script/ci-changes-detector
📚 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:
script/ci-changes-detector
⏰ 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). (5)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: lint-js-and-ruby
- GitHub Check: claude-review
🔇 Additional comments (3)
script/ci-changes-detector (3)
206-216: LGTM: Pro job determination logic mirrors non-Pro implementation.The Pro-specific job determination logic correctly mirrors the non-Pro implementation:
RUN_PRO_LINTtriggers on Pro config, Ruby, JS, or dummy changesRUN_PRO_TESTStriggers on Pro Ruby, RSpec, or JS changesRUN_PRO_DUMMY_TESTStriggers on Pro dummy, Ruby, or JS changesThis ensures Pro changes are gated appropriately while maintaining consistency with the existing non-Pro logic.
228-257: LGTM: Pro outputs correctly integrated into GitHub Actions and JSON.The Pro-specific outputs are properly integrated:
- GitHub Actions output includes
run_pro_lint,run_pro_tests, andrun_pro_dummy_tests- JSON output correctly formatted with proper comma placement
- User-facing messages clearly identify Pro-specific jobs
89-89: All referenced rakelib filenames are correct and present in the repository.The verification confirms that
rakelib/example_type.rb,rakelib/examples_config.yml, andrakelib/shakapacker_examples.rakeall exist. No issues found in line 89.
|
@CodeRabbit resolve |
✅ Actions performedComments resolved and changes approved. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/integration-tests.yml (1)
68-75: Simplify matrix exclude logic with clearer syntax and eliminate duplication across jobs.The exclude blocks in both
build-dummy-app-webpack-test-bundles(lines 68–75) anddummy-app-integration-tests(lines 161–165) are identical and use nested ternary operators that are valid in GitHub Actions but difficult to parse and maintain. The pattern conditionally sets each matrix dimension to either its target value or an empty string, which works around GitHub Actions' lack of conditional support in matrix definitions.Recommended improvements:
- Add a clarifying comment explaining that the empty-string fallback is intentional (matrix exclude requires all fields to match, so unset values prevent exclusion).
- Consider consolidating the exclude logic into a single reusable format or extracting the repeated condition to a workflow-level variable if the workflow engine supports it.
- Alternatively, generate the matrix dynamically in a preceding job and pass it via
fromJSON(per GitHub Actions best practices) if this pattern needs to be reused or becomes more complex.The parametric matrix in the first job is already cleaner than the include-based approach in the second job; applying similar patterns consistently would improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/integration-tests.yml(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
.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/integration-tests.yml
🔇 Additional comments (3)
.github/workflows/integration-tests.yml (3)
1-22: Workflow enhancements align with Pro-aware CI strategy.The workflow name clarification, paths-ignore additions for
react_on_rails_pro/**, and newforce_runinput are consistent with the broader CI modernization. The new input enables manual override of change detection, which is useful for debugging or forcing full CI runs.
90-97: Node 22 cache workaround can likely be removed—bug was fixed in v22.21.1.The V8-related crash in Node v22.21.0 was fixed in v22.21.1 (released Oct 28, 2025). The current conditional cache disable for all Node 22 versions appears overly broad. Verify which specific Node 22.x releases are tested in
matrix.node-version—if none are 22.21.0, the workaround and its comments can be removed entirely.
47-58: Verification confirms the refactored conditional logic is correct.The BASE_REF assignment uses a valid fallback chain (pull_request.base.sha → before → origin/master), and
script/ci-changes-detectorintegration is working as expected. The script properly outputs all flags (including docs_only) via internal GITHUB_OUTPUT handling, and all standard outputs are correctly exposed in the job's output definitions for downstream consumption. The PRO_* outputs generated by the script are unused in the workflow but do not cause issues.
|
@claude review this PR |
|
Claude Code is working… I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/integration-tests.yml(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
.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/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). (6)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: claude-review
🔇 Additional comments (3)
.github/workflows/integration-tests.yml (3)
10-10: Paths-ignore additions align with broader CI gating.Adding
react_on_rails_pro/**to paths-ignore for both push and pull_request triggers is consistent with the PR objective to enhance change detection and gating. This prevents unnecessary workflow runs when only Pro-related files change.Also applies to: 15-15
96-96: Node.js cache workaround for V8 bug is reasonable.The conditional cache bypass for Node 22 (
cache: ${{ matrix.node-version != '22' && 'yarn' || '' }}) is well-documented and appropriate given the noted V8 issue.Also applies to: 182-182
55-58: Review comment is based on incorrect assumptions about the workflow architecture.In PR context,
workflow_dispatchinputs likeforce_runare not available in the event payload and conditions resolve as false. However, the code correctly handles this by checkingsteps.check-label.outputs.result(which is available in PRs), not relying on the inaccessible input alone.More importantly: the detector is not being skipped for Pro filtering. Pro-related filtering happens in separate Pro workflows (
pro-integration-tests.yml,pro-lint.yml,pro-test-package-and-gem.yml), each with their owndetect-changesjob that independently handles Pro outputs. Skipping the detector inintegration-tests.ymlwhenforce_runorfull-ciis set does not prevent Pro-related outputs—Pro workflows run their own detector calls.The conditional logic is correct: when all tests should run (force_run=true or full-ci label present), set all outputs to true; otherwise, run the detector. This applies independently per workflow.
Likely an incorrect or invalid review comment.
Seeing if I can reproduce the CI results for 0b4dd06
Really just running over a lot of Chesterton's fences in order to refamiliarize myself with the codebase, especially CI.
This change is
Summary by CodeRabbit
Chores
New Features