Unify Rubocop config #2153
Unify Rubocop config #2153
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR centralizes RuboCop and development dependency configuration across the React on Rails monorepo. A root .rubocop.yml file establishes shared linting rules, while individual projects inherit from it. RuboCop pragmas are removed from code files, and rubocop gems are consolidated into a shared Gemfile. Gemfile.loader is refactored to use path-resolved evaluation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: Unify RuboCop ConfigSummaryThis PR successfully consolidates the RuboCop configuration by extracting common settings to a root .rubocop.yml file, with react_on_rails/ and react_on_rails_pro/ inheriting via inherit_from. This is excellent DRY practice and eliminates configuration drift between the two packages. Strengths
Issues and Concerns1. ERB in RuboCop Config - Potential Compatibility IssueThe react_on_rails_pro/.rubocop.yml file contains ERB template code that dynamically generates exclusion patterns from git-ignored files. RuboCops ERB support is not guaranteed across all versions and executes shell commands during config parsing, which could be slow or error-prone. Recommendation: Consider using static exclusion patterns instead, or document why ERB is necessary and test with minimum supported RuboCop version. 2. Missing Root TargetRubyVersion ValidationThe root config sets TargetRubyVersion: 3.0. Please verify this matches the gemspecs required_ruby_version and the minimum CI configuration. 3. Configuration Path TestingPer CLAUDE.md guidance on Managing File Paths, configuration changes should be tested from all locations to ensure inheritance works correctly. Testing ChecklistBefore merging, please confirm:
Overall AssessmentVerdict: Approve with minor concerns This is a solid refactoring that improves maintainability. The ERB concern is the main issue to address. Once verified that ERB works reliably and the testing checklist is complete, this is good to merge. |
react_on_rails/.rubocop.yml
Outdated
| - 'lib/generators/react_on_rails/install_generator.rb' # Generator setup methods require comprehensive error handling | ||
|
|
||
| Metrics/CyclomaticComplexity: | ||
| Max: 7 |
There was a problem hiding this comment.
This remains here because it doesn't match exactly with RORP
react_on_rails/react_on_rails_pro/.rubocop.yml
Lines 15 to 16 in 0812423
Metrics/ClassLength 150 here vs. 140 in RORP etc.)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
alexeyr-ci2
force-pushed
the
alexeyr/rubocop-config
branch
from
0812423 to
b7cfbb2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
react_on_rails_pro/.rubocop.yml (1)
6-8: ERB-based dynamic git exclusions are clever but performance-sensitive; consider CI/CD implications.RuboCop processes .rubocop.yml as ERB before YAML parsing, allowing arbitrary Ruby code embedding. Lines 6-8 execute
git status --ignored --porcelainon each RuboCop invocation to exclude git-ignored files dynamically.Pros:
- Automatically respects
.gitignorewithout manual maintenanceCons:
- Runs a shell command on every RuboCop check (performance impact, especially in large repos)
- May fail in non-git environments or shallow clones
- Could behave unexpectedly in CI/CD if git state differs from local
Consider adding error handling or an environment variable escape hatch if git state is unavailable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.prettierignore(0 hunks).rubocop.yml(1 hunks)react_on_rails/.rubocop.yml(1 hunks)react_on_rails_pro/.prettierignore(1 hunks)react_on_rails_pro/.rubocop.yml(1 hunks)
💤 Files with no reviewable changes (1)
- .prettierignore
🚧 Files skipped from review as they are similar to previous changes (2)
- react_on_rails_pro/.prettierignore
- .rubocop.yml
🧰 Additional context used
🧠 Learnings (17)
📓 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 **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
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
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: ALWAYS let Prettier and RuboCop handle ALL formatting - never manually format
📚 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:
react_on_rails_pro/.rubocop.ymlreact_on_rails/.rubocop.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 **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails_pro/.rubocop.ymlreact_on_rails/.rubocop.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:
react_on_rails_pro/.rubocop.ymlreact_on_rails/.rubocop.yml
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails_pro/.rubocop.ymlreact_on_rails/.rubocop.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 /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:
react_on_rails_pro/.rubocop.ymlreact_on_rails/.rubocop.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 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
Applied to files:
react_on_rails_pro/.rubocop.ymlreact_on_rails/.rubocop.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 sig/react_on_rails/**/*.rbs : In RBS type signature files, define type signatures that correspond to Ruby files in `lib/react_on_rails/`
Applied to files:
react_on_rails_pro/.rubocop.ymlreact_on_rails/.rubocop.yml
📚 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:
react_on_rails_pro/.rubocop.ymlreact_on_rails/.rubocop.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: Exclude `/coverage`, `/tmp`, `/gen-examples`, `/packages/react-on-rails/lib`, `/node_modules`, `/spec/dummy/node_modules`, `/spec/dummy/tmp`, `/spec/dummy/app/assets/webpack`, `/spec/dummy/log`, `/spec/dummy/e2e/playwright-report`, `/spec/dummy/test-results` from IDE indexing
Applied to files:
react_on_rails_pro/.rubocop.ymlreact_on_rails/.rubocop.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: ALWAYS let Prettier and RuboCop handle ALL formatting - never manually format
Applied to files:
react_on_rails_pro/.rubocop.ymlreact_on_rails/.rubocop.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:
react_on_rails_pro/.rubocop.ymlreact_on_rails/.rubocop.yml
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
react_on_rails_pro/.rubocop.ymlreact_on_rails/.rubocop.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 is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
react_on_rails/.rubocop.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: Runtime type checking via RBS is enabled by default in test runs; disable with `DISABLE_RBS_RUNTIME_CHECKING=true` only for rapid development iterations
Applied to files:
react_on_rails/.rubocop.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 lib/tasks/*.rake : Rails::Engine rake tasks should NOT be explicitly loaded via `rake_tasks` block; instead, place them in `lib/tasks/*.rake` where Rails::Engine automatically loads them
Applied to files:
react_on_rails/.rubocop.yml
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-10-08T20:53:47.076Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
before do
original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = original_value
end
it "tests something" do
# Your test code here
end
end
```
This practice ensures test isolation and reliability.
Applied to files:
react_on_rails/.rubocop.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). (7)
- GitHub Check: examples (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (2)
react_on_rails_pro/.rubocop.yml (1)
1-2: Root config inheritance established; verify same root file is properly configured.This file now inherits from the same root config via
../. rubocop.yml. Same verification needed asreact_on_rails/.rubocop.yml:
- Root config exists and contains all necessary cop definitions
- Root config appropriately handles the pro package's stricter or looser limits (e.g.,
Metrics/ClassLength: Max: 140here vs. 150 in the open-source package)- Performance benchmark file handling is configured
react_on_rails/.rubocop.yml (1)
1-2: Root RuboCop configuration is correctly set up and inheritance is working as expected.The root
.rubocop.ymlexists at the repository root and properly defines all necessary cop configurations includingLayout/LineLength(Max: 120),Style/StringLiterals, and others. The relative path../.rubocop.ymlinreact_on_rails/.rubocop.ymlcorrectly resolves to the root config. The spec/performance/bench.rb file will be included in RuboCop checks via the**/*.rbinclude pattern in the root config'sAllCopssection, so no additional configuration is needed.
Code ReviewSummaryThis PR successfully unifies RuboCop configuration across the monorepo by creating a root-level config that both ✅ Strengths
🔍 Issues Found1. Inconsistent AllCops Setting (Minor)Location: AllCops:
EnableNewCops: true # Note: "Enable" not "New"The root config uses Recommendation: Use 2. Missing Exclusion Pattern (Potential Bug)Location: Root The root config excludes Question: Does 3. TargetRubyVersion PrecisionLocation: Root TargetRubyVersion: 3.0The old 📋 Testing RecommendationsPer CLAUDE.md's requirements, before merging:
💡 Suggestions for Future
🎯 VerdictApprove with minor recommendations. The refactoring is sound and follows the monorepo structure correctly. The inconsistencies noted are minor and could be addressed in a follow-up if needed. The CI checks passing will confirm no regressions were introduced. Great work on reducing configuration duplication! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
react_on_rails_pro/.rubocop.yml (1)
7-9: Consider adding error handling or static fallback for the git-based ERB exclusions.The ERB pattern dynamically excludes git-ignored files, which is a valid and officially supported RuboCop feature. However, if the
git statuscommand fails (e.g., in shallow clones without git history, or CI environments without git), the entire RuboCop config fails to load with no graceful fallback. While this is unlikely in typical development or your current CI setup (GitHub Actions provides git), defensive coding would suggest either:
- Wrapping the backtick command with error handling:
<% \git status --ignored --porcelain 2>/dev/null`.lines.grep(/^!! /).each do |path| %>`- Or providing a static fallback list for environments where git is unavailable
The performance impact is minimal (git runs once per RuboCop invocation, not per file).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails_pro/.rubocop.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 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 **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
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
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: ALWAYS let Prettier and RuboCop handle ALL formatting - never manually format
📚 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:
react_on_rails_pro/.rubocop.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 **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails_pro/.rubocop.yml
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails_pro/.rubocop.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:
react_on_rails_pro/.rubocop.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 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
Applied to files:
react_on_rails_pro/.rubocop.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 /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:
react_on_rails_pro/.rubocop.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 sig/react_on_rails/**/*.rbs : In RBS type signature files, define type signatures that correspond to Ruby files in `lib/react_on_rails/`
Applied to files:
react_on_rails_pro/.rubocop.yml
📚 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:
react_on_rails_pro/.rubocop.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: ALWAYS let Prettier and RuboCop handle ALL formatting - never manually format
Applied to files:
react_on_rails_pro/.rubocop.yml
📚 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:
react_on_rails_pro/.rubocop.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:
react_on_rails_pro/.rubocop.yml
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
react_on_rails_pro/.rubocop.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: Exclude `/coverage`, `/tmp`, `/gen-examples`, `/packages/react-on-rails/lib`, `/node_modules`, `/spec/dummy/node_modules`, `/spec/dummy/tmp`, `/spec/dummy/app/assets/webpack`, `/spec/dummy/log`, `/spec/dummy/e2e/playwright-report`, `/spec/dummy/test-results` from IDE indexing
Applied to files:
react_on_rails_pro/.rubocop.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). (8)
- GitHub Check: examples (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (3)
react_on_rails_pro/.rubocop.yml (3)
5-5: Good practice:EnableNewCops: truealigns with staying current.This setting ensures new cops introduced in future RuboCop versions are enabled by default, supporting consistent linting standards.
16-42: No duplication or conflicts detected—local configuration properly extends root config.The local cop configurations in react_on_rails_pro/.rubocop.yml correctly inherit from the root config via
inherit_from: ../.rubocop.yml. The local settings (Metrics/CyclomaticComplexity, Metrics/ClassLength, RSpec/NestedGroups, RSpec/BeforeAfterAll, RSpec/MultipleDescribes, Style/GlobalVars) are unique to this subdirectory and do not duplicate any root-level configurations. The root config defines different Metrics rules (AbcSize, PerceivedComplexity, ParameterLists, MethodLength, ModuleLength) and RSpec rules without these specific exclusions, so the local overrides and exclusion lists are intentional and conflict-free.
1-2: Root.rubocop.ymlexists and is properly configured. The inheritance inreact_on_rails_pro/.rubocop.ymlis correctly set up withinherit_from: ../.rubocop.yml. The root config includes standard exclusions that adequately cover project artifacts. No configuration issues detected.Note:
spec/performance/bench.rbdoes not currently exist in the repository, so the stated PR objective cannot be verified against the actual codebase state.
alexeyr-ci2
force-pushed
the
alexeyr/rubocop-config
branch
from
c029e91 to
5ff29fb
Compare
Code Review - RuboCop Config UnificationThank you for unifying the RuboCop configuration! This is a valuable cleanup that reduces duplication. ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
react_on_rails/.rubocop.yml (1)
1-80: File must end with a newline character.The file is missing a trailing newline after line 80, which violates the coding guideline.
As per coding guidelines, add a newline at the end of the file:
RSpec/StubbedMock: Exclude: - 'spec/react_on_rails/dev/**/*_spec.rb' # Dev module tests use mixed stub/mock patterns +react_on_rails_pro/.rubocop.yml (1)
1-33: File must end with a newline character.The file is missing a trailing newline after line 33, which violates the coding guideline.
As per coding guidelines, add a newline at the end of the file:
Style/GlobalVars: Exclude: - 'spec/dummy/config/environments/development.rb' +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
react_on_rails/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.prettierignore(0 hunks).rubocop.yml(1 hunks)react_on_rails/.rubocop.yml(1 hunks)react_on_rails/Gemfile.development_dependencies(1 hunks)react_on_rails_pro/.prettierignore(1 hunks)react_on_rails_pro/.rubocop.yml(1 hunks)
💤 Files with no reviewable changes (1)
- .prettierignore
🚧 Files skipped from review as they are similar to previous changes (1)
- react_on_rails_pro/.prettierignore
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS ensure files end with a newline character before committing/pushing
Files:
react_on_rails/Gemfile.development_dependencies
🧠 Learnings (18)
📓 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 **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
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
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: ALWAYS let Prettier and RuboCop handle ALL formatting - never manually format
📚 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:
react_on_rails/Gemfile.development_dependenciesreact_on_rails/.rubocop.ymlreact_on_rails_pro/.rubocop.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 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
Applied to files:
react_on_rails/Gemfile.development_dependenciesreact_on_rails/.rubocop.ymlreact_on_rails_pro/.rubocop.yml.rubocop.yml
📚 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:
react_on_rails/Gemfile.development_dependenciesreact_on_rails/.rubocop.ymlreact_on_rails_pro/.rubocop.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:
react_on_rails/Gemfile.development_dependenciesreact_on_rails/.rubocop.ymlreact_on_rails_pro/.rubocop.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 sig/react_on_rails/**/*.rbs : In RBS type signature files, define type signatures that correspond to Ruby files in `lib/react_on_rails/`
Applied to files:
react_on_rails/Gemfile.development_dependenciesreact_on_rails/.rubocop.ymlreact_on_rails_pro/.rubocop.yml.rubocop.yml
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails/Gemfile.development_dependenciesreact_on_rails/.rubocop.ymlreact_on_rails_pro/.rubocop.yml.rubocop.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:
react_on_rails/Gemfile.development_dependenciesreact_on_rails/.rubocop.ymlreact_on_rails_pro/.rubocop.yml.rubocop.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 **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails/Gemfile.development_dependenciesreact_on_rails/.rubocop.ymlreact_on_rails_pro/.rubocop.yml.rubocop.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 is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
react_on_rails/.rubocop.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: ALWAYS let Prettier and RuboCop handle ALL formatting - never manually format
Applied to files:
react_on_rails/.rubocop.ymlreact_on_rails_pro/.rubocop.yml.rubocop.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: Exclude `/coverage`, `/tmp`, `/gen-examples`, `/packages/react-on-rails/lib`, `/node_modules`, `/spec/dummy/node_modules`, `/spec/dummy/tmp`, `/spec/dummy/app/assets/webpack`, `/spec/dummy/log`, `/spec/dummy/e2e/playwright-report`, `/spec/dummy/test-results` from IDE indexing
Applied to files:
react_on_rails/.rubocop.yml.rubocop.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:
react_on_rails/.rubocop.yml
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
react_on_rails/.rubocop.ymlreact_on_rails_pro/.rubocop.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: Runtime type checking via RBS is enabled by default in test runs; disable with `DISABLE_RBS_RUNTIME_CHECKING=true` only for rapid development iterations
Applied to files:
react_on_rails/.rubocop.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 lib/tasks/*.rake : Rails::Engine rake tasks should NOT be explicitly loaded via `rake_tasks` block; instead, place them in `lib/tasks/*.rake` where Rails::Engine automatically loads them
Applied to files:
react_on_rails/.rubocop.yml
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-10-08T20:53:47.076Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
before do
original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = original_value
end
it "tests something" do
# Your test code here
end
end
```
This practice ensures test isolation and reliability.
Applied to files:
react_on_rails/.rubocop.yml
📚 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:
react_on_rails_pro/.rubocop.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). (8)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: claude-review
🔇 Additional comments (3)
.rubocop.yml (1)
1-99: Root configuration structure looks solid.The shared RuboCop configuration provides a good baseline with reasonable cop settings, metrics thresholds, and RSpec configurations that both packages can inherit from.
react_on_rails/.rubocop.yml (1)
1-2: Good consolidation via inheritance.Switching to inherit from the root configuration is a clean approach that reduces duplication while maintaining package-specific exceptions where needed.
react_on_rails_pro/.rubocop.yml (1)
1-2: Inheritance approach reduces duplication effectively.Inheriting from the root config while maintaining only pro-specific exclusions is a clean consolidation.
Code Review for PR #2153: Unify Rubocop configOverviewThis PR successfully centralizes RuboCop configuration using inheritance, reducing duplication across the monorepo. ✅ Strengths
|
The correct RuboCop syntax is 'NewCops: enable' under AllCops, which is already defined in the root .rubocop.yml that this config inherits from. 'EnableNewCops: true' is not a valid RuboCop configuration key and will be silently ignored. Co-authored-by: Ihab Adham <ihabadham@users.noreply.github.com>
Code Review - PR #2153: Unify Rubocop ConfigSummaryThis PR centralizes RuboCop configuration by creating a root-level .rubocop.yml that is inherited by both react_on_rails/ and react_on_rails_pro/ packages. This is a well-executed refactoring that reduces duplication and improves maintainability. Strengths
Observations and Questions1. Pro Package ERB Templating - The Pro package uses dynamic exclusions via ERB (lines 6-8). Is this approach documented? It's clever but makes config non-standard YAML. Consider adding explanatory comment or documenting in CLAUDE.md. 2. Version Consistency - Root config sets TargetRubyVersion: 3.0, but CLAUDE.md shows project supports Ruby 3.2+ (min) and 3.4 (latest). Should this be updated to 3.2? 3. Metrics Loosening - Root config has Metrics/CyclomaticComplexity Max: 8 and NestedGroups Max: 5, but react_on_rails previously had Max: 7 and Max: 4 (stricter). This loosens limits for open-source package. Is this intentional? Security and Quality✅ No security concerns - configuration changes only Final VerdictAPPROVED with minor questions ✅ This is high-quality refactoring that reduces maintenance burden, improves consistency, follows best practices, and maintains package-specific flexibility. Questions raised are minor clarifications, not blockers. Ready to merge once CI passes. Suggested Follow-ups
Great work on this refactoring! 🎉 |
Co-authored-by: Ihab Adham <ihabadham@users.noreply.github.com>
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)
react_on_rails_pro/.rubocop.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 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 **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: ALWAYS let Prettier and RuboCop handle ALL formatting - never manually format
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
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Resolve merge conflicts by: resolving logical conflicts first, verifying file paths with grep, testing affected scripts, auto-fixing formatting with `rake autofix`, and testing critical scripts if build configs changed
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
📚 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 **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails_pro/.rubocop.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:
react_on_rails_pro/.rubocop.yml
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails_pro/.rubocop.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 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
Applied to files:
react_on_rails_pro/.rubocop.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:
react_on_rails_pro/.rubocop.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 sig/react_on_rails/**/*.rbs : In RBS type signature files, define type signatures that correspond to Ruby files in `lib/react_on_rails/`
Applied to files:
react_on_rails_pro/.rubocop.yml
📚 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:
react_on_rails_pro/.rubocop.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 /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:
react_on_rails_pro/.rubocop.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: ALWAYS let Prettier and RuboCop handle ALL formatting - never manually format
Applied to files:
react_on_rails_pro/.rubocop.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: Exclude `/coverage`, `/tmp`, `/gen-examples`, `/packages/react-on-rails/lib`, `/node_modules`, `/spec/dummy/node_modules`, `/spec/dummy/tmp`, `/spec/dummy/app/assets/webpack`, `/spec/dummy/log`, `/spec/dummy/e2e/playwright-report`, `/spec/dummy/test-results` from IDE indexing
Applied to files:
react_on_rails_pro/.rubocop.yml
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
react_on_rails_pro/.rubocop.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.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (2)
react_on_rails_pro/.rubocop.yml (2)
24-28: Retain custom RSpec language expectations for react_on_rails testing.The custom expectation matchers
change_text_expect_dom_selectorandcheck_matchare specific to react_on_rails' testing framework and should remain in this configuration. Keeping them in the Pro package config (rather than centralizing in the root config) is appropriate since these are Pro-specific test helpers.
6-8: ERB preprocessing in RuboCop configuration is valid.Configuration files are pre-processed using the ERB templating mechanism. This makes it possible to add dynamic content that will be evaluated when the configuration file is read. The pattern of using
git status --ignoredto dynamically exclude git-ignored paths is a valid example of letting RuboCop ignore all files ignored by Git, so the implementation here is correct.
ihabadham
left a comment
ihabadham
left a comment
There was a problem hiding this comment.
just some comments for failing CI
.rubocop.yml
Outdated
| - 'node_modules/**/*' | ||
| - 'spec/fixtures/**/*' | ||
| - 'spec/react_on_rails/dummy-for-generators/**/*' | ||
| - 'vendor/**/*' |
There was a problem hiding this comment.
**/vendor/**/* should be used instead of vendor/**/* because CI installs gems to react_on_rails/vendor/bundle/ and react_on_rails_pro/vendor/bundle/. Without the **/ prefix, RuboCop scans those directories and finds .rubocop.yml files inside gems (like docile which references panolint, and cypress-on-rails which references rubocop-rails-omakase) - causing the CI failures.
|
@claude can you address my review i just made by: if any if these steps break or behave unexpected, stop and report back |
|
Claude finished @ihabadham's task —— View job Addressing Review Feedback - Status ReportTasks Completed
Current Status: Needs Approval for Auto-CorrectBoth claimed issues were VALID and have been fixed:
CI Results: The "Lint JS and Ruby" job failed with 12 autocorrectable offenses: All 12 offenses are
Next Steps: I need approval to run: cd react_on_rails && bundle exec rubocop --auto-correct-allThis will remove the 12 redundant disable comments. Then I can commit, push, and wait for CI to pass before proceeding with the rubocop-rails-omakase revert test. Commits Pushed: |
Change vendor/**/* to **/vendor/**/* to properly exclude vendor directories in subdirectories like react_on_rails/vendor/bundle/ and react_on_rails_pro/vendor/bundle/ where CI installs gems. Co-authored-by: Ihab Adham <ihabadham@users.noreply.github.com>
Without inherit_mode merge, child Exclude arrays replace the parent's instead of merging with them. This was causing all root exclusions (node_modules, tmp, coverage, etc.) to be lost in the child configs. Co-authored-by: Ihab Adham <ihabadham@users.noreply.github.com>
@alexeyr-ci2 any reason for this? or just globalize the pro configs? |
Code Review - RuboCop Configuration Unification✅ Overall AssessmentThis PR successfully unifies RuboCop configuration across the monorepo by creating a shared root configuration that both 🎯 Strengths
📋 Code Quality ObservationsRemoved RuboCop Disables - All removed disables appear justified:
Configuration Structure:
🔍 Potential Concerns
🧪 Testing RecommendationsBased on CLAUDE.md requirements:
🔧 Minor Suggestions
📚 Documentation
🚀 RecommendationApprove with minor follow-ups:
This is a solid refactoring that improves maintainability. The code quality improvements (removing unnecessary disable comments) suggest the original suppressions were overly cautious. 📊 Stats
|
Code Review - PR #2153: Unify RuboCop Config✅ Overall AssessmentThis is a well-executed refactoring that successfully centralizes RuboCop configuration across the monorepo. The changes follow best practices for inheritance-based configuration management. 🎯 Strengths
🔍 Areas to Verify
🧪 Testing RecommendationsMANDATORY (per CLAUDE.md): # Verify RuboCop passes with zero offenses
bundle exec rubocop
# Test from both sub-packages
cd react_on_rails && bundle exec rubocop
cd react_on_rails_pro && bundle exec rubocopAdditional verification: # Verify the inheritance is working correctly
cd react_on_rails
bundle exec rubocop --show-cops | grep -A2 "Metrics/CyclomaticComplexity"
# Should show: Max: 8 (from root config)📋 Code Quality
🎓 Suggestions
✅ Approval RecommendationAPPROVE with minor follow-up recommended. This is a solid refactoring that improves maintainability. The minor issues noted are not blockers and can be addressed in a follow-up PR if needed. The centralized configuration will make it much easier to update linting rules consistently across the monorepo in the future. Review performed according to CLAUDE.md guidelines for React on Rails project. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.rubocop.yml (1)
7-19: AllCops Include overrides defaults and drops subdirectory GemfilesBy setting
AllCops: Includeexplicitly, you replace RuboCop’s default include patterns. As written, only the rootGemfileis picked up, notreact_on_rails/**/Gemfile,react_on_rails_pro/**/Gemfile, or other special files (Guardfile/Capfile, etc.). If you still want those to be linted, consider adding**/Gemfile(and any other patterns you care about) back intoInclude, or avoid overriding the defaults entirely.Aside from this, the consolidated config (extensions, excludes, metrics, RSpec cops) looks consistent with the repo’s style goals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
react_on_rails_pro/Gemfile.lockis excluded by!**/*.lockreact_on_rails_pro/spec/dummy/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.rubocop.yml(1 hunks)Gemfile.shared_dev_dependencies(1 hunks)react_on_rails/Gemfile.development_dependencies(1 hunks)react_on_rails_pro/Gemfile(1 hunks)react_on_rails_pro/Gemfile.development_dependencies(1 hunks)
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.{rb,js,ts,jsx,tsx,json,yml,yaml,md} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.rb : ALWAYS let RuboCop handle ALL formatting - never manually format Ruby code
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.rbs : Add RBS signature files for all new Ruby files in `lib/react_on_rails/` and register them in Steepfile; validate with `bundle exec rake rbs:validate`
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to react_on_rails_pro/**/*.{js,ts,jsx,tsx,json,css,scss} : The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration separate from the root; CI lints both directories separately
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to react_on_rails_pro/**/*.{js,ts,jsx,tsx,json,css,scss} : The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration separate from the root; CI lints both directories separately
Applied to files:
Gemfile.shared_dev_dependencies.rubocop.ymlreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.{rb,js,ts,jsx,tsx,json,yml,yaml,md} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
Gemfile.shared_dev_dependencies.rubocop.ymlreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails/Gemfile.development_dependencies
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to lib/react_on_rails/**/*.rb : Engine code has limited access to host application; use `Rails.root` for host app paths and `ReactOnRails.configure` for engine configuration
Applied to files:
Gemfile.shared_dev_dependencies.rubocop.ymlreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile
📚 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:
Gemfile.shared_dev_dependencies.rubocop.ymlreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile
📚 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:
Gemfile.shared_dev_dependencies.rubocop.ymlreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile
📚 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:
Gemfile.shared_dev_dependenciesreact_on_rails/Gemfile.development_dependencies
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to lib/generators/react_on_rails/**/*.rb : Generators run in host app context, not engine context; do not assume host app structure (e.g., `app/javascript/` may not exist in older apps)
Applied to files:
Gemfile.shared_dev_dependencies.rubocop.ymlreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.rbs : Add RBS signature files for all new Ruby files in `lib/react_on_rails/` and register them in Steepfile; validate with `bundle exec rake rbs:validate`
Applied to files:
Gemfile.shared_dev_dependencies.rubocop.ymlreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.rb : ALWAYS let RuboCop handle ALL formatting - never manually format Ruby code
Applied to files:
.rubocop.ymlreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails/Gemfile.development_dependencies
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to lib/react_on_rails/**/*.rb : Create corresponding RBS signature files in `sig/react_on_rails/` for new Ruby files and add them to Steepfile for type checking
Applied to files:
.rubocop.ymlreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
.rubocop.ymlreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.{rb,js,ts,jsx,tsx,json,yml,yaml,md} : ALWAYS ensure files end with a newline character before committing
Applied to files:
.rubocop.yml
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to spec/dummy/**/*.{spec,test}.{js,ts} : Playwright E2E tests use `cypress-on-rails` gem for Rails integration; use helpers like `app('clean')`, `appEval()`, and `appFactories()` for test setup instead of UI automation
Applied to files:
.rubocop.ymlreact_on_rails_pro/Gemfile
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to spec/dummy/e2e/playwright/app_commands/**/*.rb : Create custom app commands for Playwright E2E tests in this directory using the `command` syntax provided by `cypress-on-rails` gem
Applied to files:
.rubocop.yml
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for open-source features, bug fixes, breaking changes, deprecations, and performance improvements using format: `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)`
Applied to files:
react_on_rails_pro/Gemfile.development_dependenciesreact_on_rails/Gemfile.development_dependencies
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Use RBS (Ruby Signature) with Steep for static type checking; enable runtime type checking by default during test runs (can be disabled with `DISABLE_RBS_RUNTIME_CHECKING=true`)
Applied to files:
react_on_rails_pro/Gemfile.development_dependenciesreact_on_rails/Gemfile.development_dependencies
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
react_on_rails_pro/Gemfile
⏰ 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). (12)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: precompile-check
- GitHub Check: claude-review
🔇 Additional comments (4)
react_on_rails_pro/Gemfile.development_dependencies (1)
4-6: Clarifying comment about shared dev dependencies looks goodThe note about loading shared dev dependencies via the main Gemfile (due to
Gemfile.loader’s use ofevaland__dir__) is clear and should help prevent future confusion. No changes needed.Gemfile.shared_dev_dependencies (1)
1-10: Centralized RuboCop dev dependencies look consistentDefining
rubocop,rubocop-performance, andrubocop-rspeconce in this shared file under:development, :testis a clean way to keep linting tool versions in sync betweenreact_on_railsandreact_on_rails_pro.Please just double‑check that these pinned versions match what CI currently expects so Bundler/lockfiles don’t drift after merge.
react_on_rails_pro/Gemfile (1)
8-11: Ordering and path for shared dev deps eval look correctLoading
../Gemfile.shared_dev_dependencieshere (beforeGemfile.loader) aligns with the__dir__comment and should keep shared RuboCop tooling available for the pro app without changing runtime dependencies.Worth a quick Bundler run in CI to confirm there are no surprises in group resolution, but the structure itself looks sound.
react_on_rails/Gemfile.development_dependencies (1)
3-3: Shared dev deps inclusion from engine Gemfile.development_dependencies LGTMPulling in
../Gemfile.shared_dev_dependenciesat the top centralizes RuboCop-related tooling while keeping the existing development/test groups intact. This should reduce version skew between the engine and the pro app.No changes requested; just ensure CI/Bundler runs pick up the new shared file as expected.
Code Review: Unify RuboCop ConfigThank you for this PR! This is a well-structured refactoring that centralizes RuboCop configuration across the monorepo. Here's my detailed review: ✅ Strengths
|
alexeyr-ci2
force-pushed
the
alexeyr/rubocop-config
branch
from
085b946 to
a04fa6a
Compare
| # Keep in sync with https://github.com/shakacode/react_on_rails/blob/master/Gemfile.development_dependencies | ||
| # frozen_string_literal: true | ||
|
|
||
| # NOTE: Shared dev dependencies (rubocop, etc.) are loaded via eval_gemfile in |
There was a problem hiding this comment.
Couldn't we fix Gemfile.loader instead? Changing eval(base_deps) to eval(base_deps, binding, filepath) would preserve __dir__, and we wouldn't have this here look inconsistent between ror and rorp
…ution Changed eval() to instance_eval(content, filepath) which preserves __dir__ in evaluated content (matching Bundler's internal eval_gemfile pattern). This allows eval_gemfile calls to work correctly inside loaded files, so shared dev dependencies can now be loaded from Gemfile.development_dependencies where they logically belong, instead of requiring a workaround in the main Gemfile. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
react_on_rails_pro/Gemfile.loader (1)
8-16: Minor optimization opportunity: redundant path expansion.
File.expand_pathis called twice for each path—once in the existence check and once for assignment. This could be streamlined:-override_deps_path = if ENV["CI"] == "true" && File.exist?(File.expand_path("./Gemfile.ci", __dir__)) +ci_path = File.expand_path("./Gemfile.ci", __dir__) +local_path = File.expand_path("./Gemfile.local", __dir__) + +override_deps_path = if ENV["CI"] == "true" && File.exist?(ci_path) # In CI environment, use CI dependencies - File.expand_path("./Gemfile.ci", __dir__) -elsif File.exist?(File.expand_path("./Gemfile.local", __dir__)) + ci_path +elsif File.exist?(local_path) # In non-CI environment, use local dependencies if they exist - File.expand_path("./Gemfile.local", __dir__) + local_path endNot critical—current code is functionally correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react_on_rails_pro/Gemfile.development_dependencies(1 hunks)react_on_rails_pro/Gemfile.loader(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.{rb,js,ts,jsx,tsx,json,yml,yaml,md} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to react_on_rails_pro/**/*.{js,ts,jsx,tsx,json,css,scss} : The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration separate from the root; CI lints both directories separately
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.rbs : Add RBS signature files for all new Ruby files in `lib/react_on_rails/` and register them in Steepfile; validate with `bundle exec rake rbs:validate`
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.rb : ALWAYS let RuboCop handle ALL formatting - never manually format Ruby code
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to lib/react_on_rails/**/*.rb : Engine code has limited access to host application; use `Rails.root` for host app paths and `ReactOnRails.configure` for engine configuration
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: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to lib/react_on_rails/**/*.rb : Create corresponding RBS signature files in `sig/react_on_rails/` for new Ruby files and add them to Steepfile for type checking
📚 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:
react_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile.loader
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to lib/react_on_rails/**/*.rb : Create corresponding RBS signature files in `sig/react_on_rails/` for new Ruby files and add them to Steepfile for type checking
Applied to files:
react_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile.loader
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to react_on_rails_pro/**/*.{js,ts,jsx,tsx,json,css,scss} : The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration separate from the root; CI lints both directories separately
Applied to files:
react_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile.loader
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.rbs : Add RBS signature files for all new Ruby files in `lib/react_on_rails/` and register them in Steepfile; validate with `bundle exec rake rbs:validate`
Applied to files:
react_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile.loader
📚 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:
react_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile.loader
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for open-source features, bug fixes, breaking changes, deprecations, and performance improvements using format: `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)`
Applied to files:
react_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile.loader
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to lib/react_on_rails/**/*.rb : Engine code has limited access to host application; use `Rails.root` for host app paths and `ReactOnRails.configure` for engine configuration
Applied to files:
react_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile.loader
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to lib/generators/react_on_rails/**/*.rb : Generators run in host app context, not engine context; do not assume host app structure (e.g., `app/javascript/` may not exist in older apps)
Applied to files:
react_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/Gemfile.loader
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.{rb,js,ts,jsx,tsx,json,yml,yaml,md} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails_pro/Gemfile.development_dependencies
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Applies to **/*.rb : ALWAYS let RuboCop handle ALL formatting - never manually format Ruby code
Applied to files:
react_on_rails_pro/Gemfile.development_dependencies
📚 Learning: 2025-12-10T09:27:45.093Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T09:27:45.093Z
Learning: Use RBS (Ruby Signature) with Steep for static type checking; enable runtime type checking by default during test runs (can be disabled with `DISABLE_RBS_RUNTIME_CHECKING=true`)
Applied to files:
react_on_rails_pro/Gemfile.development_dependencies
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails_pro/Gemfile.loader
⏰ 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: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: precompile-check
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (3)
react_on_rails_pro/Gemfile.development_dependencies (1)
4-6: Clean centralization of shared development dependencies.Using
eval_gemfilewith the resolved path correctly loads the shared rubocop configuration from the repository root. This approach maintains consistency across the monorepo while keeping the path resolution explicit and correct.react_on_rails_pro/Gemfile.loader (2)
4-5: Good refactoring to capture the path for later reuse.Storing the expanded path in a variable enables passing it to
instance_eval, which preserves__dir__context in the evaluated content.
30-35: Correctly addresses the__dir__preservation concern.Using
instance_eval(content, filepath)preserves the__dir__context in evaluated content, allowingeval_gemfilecalls withinGemfile.development_dependenciesto resolve paths correctly relative to their source file. This addresses the past review feedback about maintaining consistent path resolution.
Code Review: Unify RuboCop ConfigSummaryThis PR successfully consolidates RuboCop configuration across the monorepo by creating a root config that both react_on_rails/ and react_on_rails_pro/ inherit from. This is a solid refactoring that reduces duplication and ensures consistency. Strengths
Issues and ConcernsCritical: Gemfile.loader Implementation ChangeLocation: react_on_rails_pro/Gemfile.loader lines 29-35 The change from eval() to instance_eval() is potentially problematic. instance_eval changes self to the current object, which could affect gem declarations. The comment says this preserves dir in evaluated content, but this needs verification. Questions:
Recommendation: Document why instance_eval is necessary and verify CI passes with this change. Minor: RuboCop Version JumpOpen source: rubocop 1.61.0 This is good unification, but jumping 25 minor versions is significant. The PR handles this well by removing inline disable comments that are no longer needed. Question: Were there any newly-failing cops that required code changes beyond removing unnecessary disables? Minor: Pro RuboCop config has ERBThe Pro config uses ERB to dynamically exclude git-ignored files. This is interesting but potentially risky: Pros:
Cons:
Recommendation: Consider whether this dynamic exclusion is worth the complexity. Minor: RSpec metadata changeLocation: react_on_rails_pro/spec/dummy/spec/rails_helper.rb lines 129, 133 Changed from config.before(:each, type: :system, js: true) to config.before(:each, :js, type: :system). This is likely a RuboCop auto-fix. The new style is more idiomatic, but make sure these tests still run correctly. Testing RecommendationsBefore merging, verify:
Documentation and ChangelogPer CLAUDE.md, this is internal tooling improvement, so no changelog entry needed. Overall AssessmentThis is a well-executed refactoring that follows good monorepo practices. Main concerns are:
Approval recommendation: Approve with minor verification Once CI passes (especially pro-lint-js-and-ruby and Pro integration tests), this is ready to merge. Great work on consolidating the configs! |
|
@coderabbitai It isn't clear what changes are still requested. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3914 🏁 Script executed: Length of output: 99079 @alexeyr-ci2, based on my analysis of the PR state: Current Status ✅All requested work appears to be complete:
Open Question (Non-blocking)The only outstanding item is @ihabadham's question from earlier:
This was regarding the Summary: No changes are currently requested. The PR is approved and ready to merge once you address ihabadham's question about the complexity threshold rationale (if you'd like to provide that context for the record). |
|
@coderabbitai Your review status is "coderabbitai[bot] Requested changes" which is blocking the merge. |
|
@alexeyr-ci2, you're absolutely right—I apologize for blocking the merge! All the requested changes have been completed: ✅ RuboCop offenses fixed I'm approving this PR now to unblock the merge. ✅ Actions performedComments resolved and changes approved. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 158 🏁 Script executed: Length of output: 2397 🏁 Script executed: Length of output: 11881 @alexeyr-ci2, I've approved the PR! ✅ Status Update:
The "Request changes" review that was blocking the merge has been replaced with an approval. You can now proceed with merging the PR. |
Code Review - PR #2153: Unify RuboCop ConfigSummaryThis PR successfully consolidates RuboCop configuration across the monorepo by creating a shared root config that both react_on_rails/ and react_on_rails_pro/ inherit from. This is a solid refactor that reduces duplication and ensures consistency. Strengths
Code Quality & Best PracticesConfiguration Structure:
Metrics Changes:
Potential Issues1. rubocop-rails-omakase dependency (Commit 5ff29fb)Concern: The commit message says "no idea why CI wants it" - this suggests uncertainty about why this dependency was added. Questions:
Recommendation: Verify if this is truly needed. If CI doesn't actually fail without it, remove it to keep dependencies lean. 2. Gemfile.* wildcard exclusionIn root config, Naming/FileName excludes '**/Gemfile.*' Potential issue: This will also exclude Gemfile.lock from filename checks. While Gemfile.lock doesn't typically trigger this cop, using '**/Gemfile.*' is very broad. Recommendation: Consider being more specific with explicit file names, though the current approach is more maintainable if you add more Gemfile.* variants in the future. Recommendations
Approval StatusOverall: Looks good with minor suggestions The core refactor is solid and achieves the stated goal. The main uncertainty is the rubocop-rails-omakase dependency. If that's intentional and needed for your CI environment, this is ready to merge. Once CI passes and the omakase dependency is confirmed intentional, this PR will:
Great work on the refactor! |
Summary
Extracted from #1868 because it adds
spec/performance/bench.rband the default Rubocop config doesn't fit our repo.Pull Request checklist
Add/update test to cover these changesUpdate documentationUpdate CHANGELOG fileSummary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.