Keep root RuboCop bundle lint-only#3841
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR separates the root lint/dev Gemfile from package Gemfiles, updates Dependabot to include the repo root, adds bundler helpers and per-Gemfile execution in setup/CI scripts, delegates package rake tasks from the root Rakefile, rewires lint tasks to use the root bundle, and standardizes release flag parsing. ChangesBundler Environment Split for Lint-Only Root Bundle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 ReviewOverviewThis PR cleanly separates the monorepo root bundle from the OSS package bundle, shrinking the root Issues Found1.
2.
3. The ~70-entry array is the right trade-off to avoid loading the package bundle into the root context, but it will silently fail to delegate any task added to 4. The new helper looks correct for Positive Notes
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bin/ci-local (1)
240-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse explicit root Gemfile for root dependency bootstrap.
Line 242 still runs bare
bundle install. If the caller environment already hasBUNDLE_GEMFILEset, this can install/update the wrong bundle and undermine the root-vs-package boundary this PR introduces.Suggested fix
-if [ ! -d "node_modules" ] || [ ! -d "vendor/bundle" ]; then +if [ ! -d "node_modules" ] || ! bundle_command "$ROOT_GEMFILE" bundle check >/dev/null 2>&1; then echo -e "${YELLOW}Installing dependencies...${NC}" - if ! (bundle install && pnpm install); then + if ! (bundle_command "$ROOT_GEMFILE" bundle install && pnpm install); then echo -e "${RED}✗ Dependency installation failed${NC}" echo -e "${RED}Cannot proceed without dependencies. Please fix the errors above.${NC}" exit 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/ci-local` around lines 240 - 243, The script currently runs a bare `bundle install` in bin/ci-local which can pick up a caller's BUNDLE_GEMFILE and install/update the wrong bundle; update the installation step to explicitly target the repository root Gemfile by invoking bundle with an explicit Gemfile (e.g., set BUNDLE_GEMFILE=Gemfile or pass --gemfile=Gemfile) when running the `bundle install` command so the root dependency bootstrap always uses the intended Gemfile.AGENTS.md (1)
116-116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerify bundle selection for package spec files.
With the root bundle now limited to lint/tooling (plus RSpec only for benchmark specs), running
bundle exec rspec react_on_rails/spec/react_on_rails/...from the repo root would use the root bundle, which lacks the package's test dependencies. Consider updating this example to match the delegation pattern:(cd react_on_rails && bundle exec rspec spec/react_on_rails/path/to/spec.rb)This ensures the package bundle (which owns test dependencies per the PR objectives) is used for package tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` at line 116, Update the example command in AGENTS.md that currently shows "bundle exec rspec react_on_rails/spec/react_on_rails/path/to/spec.rb" so it runs the package's bundle instead of the repo root bundle; replace it with a delegated invocation that cd's into the react_on_rails package (using a subshell or equivalent) and then runs bundle exec rspec on the package-local spec path (spec/react_on_rails/path/to/spec.rb) to ensure the package's test dependencies are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@AGENTS.md`:
- Line 116: Update the example command in AGENTS.md that currently shows "bundle
exec rspec react_on_rails/spec/react_on_rails/path/to/spec.rb" so it runs the
package's bundle instead of the repo root bundle; replace it with a delegated
invocation that cd's into the react_on_rails package (using a subshell or
equivalent) and then runs bundle exec rspec on the package-local spec path
(spec/react_on_rails/path/to/spec.rb) to ensure the package's test dependencies
are used.
In `@bin/ci-local`:
- Around line 240-243: The script currently runs a bare `bundle install` in
bin/ci-local which can pick up a caller's BUNDLE_GEMFILE and install/update the
wrong bundle; update the installation step to explicitly target the repository
root Gemfile by invoking bundle with an explicit Gemfile (e.g., set
BUNDLE_GEMFILE=Gemfile or pass --gemfile=Gemfile) when running the `bundle
install` command so the root dependency bootstrap always uses the intended
Gemfile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e42514d-c3d8-4c31-b0a9-3503b6045951
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/dependabot.ymlAGENTS.mdGemfileRakefilebin/ci-localbin/ci-rerun-failuresbin/setupconductor-setup.shrakelib/release.rakereact_on_rails/rakelib/lint.rake
Greptile SummaryThis PR separates the monorepo root
Confidence Score: 3/5The core Gemfile split and Rake delegation are correct, but bin/ci-local uses bare Every new run_job call for OSS Ruby tests uses bin/ci-local — all four new run_job commands wrapping OSS bundle tasks need their Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["bundle exec rake task\n(repo root)"] --> B{task type?}
B -->|"lint / lint:rubocop"| C["root_bundle_exec_in\nBUNDLE_GEMFILE=root Gemfile\nrubocop . in gem_root"]
B -->|"delegated tasks\nnode_package, run_rspec:*, etc."| D["package_rake_task\nDir.chdir(gem_root)"]
D --> E["ensure_package_bundle\nbundle check / install"]
E --> F["Bundler.with_unbundled_env\nbundle exec rake task"]
F --> G["react_on_rails/Rakefile\n(package bundle)"]
B -->|"release / sync_github_release"| H["rakelib/release.rake\n(root bundle)"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issue.
Reviewed by Cursor Bugbot for commit e051b25. Configure here.
Code ReviewGood architectural split — keeping the root bundle focused on tooling-only gems rather than pulling in the full Rails/RoR stack is a clear improvement. The lock file shrink alone is a strong signal. A few items worth addressing before merge: Delegation list will drift (Rakefile lines 86–152). The 65-entry hardcoded task list is the main maintainability risk. Any new task added to the package Rakefile must also be manually mirrored here. When a task is added to the package but not the root,
Minor positives: The |
Code ReviewOverviewThis PR cleanly separates the monorepo root Ruby bundle from the OSS gem bundle, shrinking the root FindingsMedium — The pre-pass loop at line 353 checks Low —
Low — Replacing Low — Root-level Positives
Overall this is a well-structured change with good validation coverage. The |
Pro Node Renderer Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
|
Code Review PR 3841: Keep root RuboCop bundle lint-only |
Issues and Suggestions
|
|
Code Review - PR 3841: Keep root RuboCop bundle lint-only Overview This PR cleanly separates the monorepo root Ruby bundle from the OSS gem bundle. Previously the root Gemfile used eval_gemfile to pull in the entire Rails dependency tree; now it only contains lint/hook/release/benchmark tooling (~10 gems vs ~100). The delegation approach in the root Rakefile is elegant: root rake task shells out with bundle exec rake task inside the package directory, so the surface API is unchanged. What is Good
Issues and Suggestions 1. bundle_command is duplicated across three scripts The same ~10-line function is copy-pasted into bin/ci-local, bin/ci-rerun-failures, and bin/setup, with a comment asking them to stay in sync. Drift here will be a silent bug (wrong Gemfile selected). Consider extracting to a sourced helper, e.g. bin/lib/bundle-helpers.sh, and sourcing it from each script. 2. Static delegation list is a maintenance footprint The task delegation block in Rakefile is ~70 lines that must be manually kept in sync with the package Rakefile. The audit hint in the comment is helpful but easy to miss when adding new tasks. A CI check that diffs root vs package task lists would make drift visible automatically. 3. @package_bundle_ready on the main object Using @package_bundle_ready at top-level scope works (it is an ivar on Ruby main object) and is safe for single-threaded Rake. It is a subtle pattern -- future contributors may not recognize it. Low priority. 4. release_truthy? -- nil handling is correct, but worth a test ENV.fetch returning nil when absent means release_truthy?(nil) correctly returns false. The existing ReactOnRails::Utils.object_to_boolean also handles falsy inputs explicitly. Looks correct on audit -- just ensure benchmark specs cover this function if not already tested. 5. Root rake lint behavior change may surprise contributors Previously rake lint ran the full package lint (RuboCop + ESLint + Prettier + Stylelint). Now it runs root-bundle RuboCop only and prints a hint. The AGENTS.md update is good. Consider making the task output more prominent (e.g. print the new recommended command in yellow/bold) so contributors who run rake lint by muscle memory immediately understand the scope change. 6. _run_job_shell subshell scope change Wrapping eval in a subshell (eval) correctly scopes cd calls to the job, which is the intent. Side effect: any export or variable assignments in the command no longer propagate to subsequent jobs. Given the jobs being run this is the right tradeoff, but worth keeping in mind if future jobs rely on inter-job env propagation. Minor / Nits
Overall Assessment Approve with suggestions. The architecture is sound and the implementation is correct. The main ask is to consider extracting bundle_command to a shared sourced helper to avoid silent drift risk across the three CI scripts. The task delegation list maintainability concern is worth tracking as a follow-up issue rather than blocking this PR. |
Core Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Pro (shard 1/2) Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Pro (shard 2/2) Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
1 participant
Summary
Closes #3837.
react_on_railsbundle so root callers keep working without contaminating package locks.Validation
bundle installbundle exec rake lint(cd react_on_rails && bundle exec rake lint:rubocop)with package/root lockfile hashes unchangedbundle exec rubocop Rakefile Gemfile rakelib/release.rake react_on_rails/rakelib/lint.rakebundle exec ruby -c Rakefileruby -c rakelib/release.rakeruby -c react_on_rails/rakelib/lint.rakebundle exec rspec benchmarks/speczsh -n conductor-setup.shbash -n bin/ci-local bin/ci-rerun-failures bin/setupshellcheck bin/ci-rerun-failures bin/ci-local bin/setuppnpm exec prettier --check AGENTS.md .github/dependabot.ymlbundle exec bin/setup --helpbundle exec rake node_package(cd react_on_rails && bundle exec rake rbs:validate)(cd react_on_rails && bundle check)(cd react_on_rails && BUNDLE_GEMFILE="$PWD/Gemfile" bundle exec rspec spec/react_on_rails/ruby_version_support_spec.rb)(cd react_on_rails && BUNDLE_GEMFILE=../Gemfile bundle exec rubocop --format simple --cache false -- Rakefile ../Rakefile ../rakelib/release.rake)codex review --base origin/maingit diff --checkFull CI and benchmark are requested because this changes root/package Rake delegation and setup/CI bundle selection.
Note
Medium Risk
Touches root vs package bundle boundaries, Rake delegation, and local CI/setup scripts; wrong Gemfile selection could break contributor workflows, but runtime gem behavior is unchanged.
Overview
Splits the monorepo root Ruby bundle from the OSS gem so
bundle installat the repo root only pulls lint, hooks, release, and benchmark-spec tooling (RuboCop, Rake, RSpec, lefthook, etc.), instead of evaluatingreact_on_rails/Gemfileand dragging in Rails and the full test stack.Gemfile.lockshrinks accordingly; Dependabot’s bundler config now includes/for that lock.Root
rakestill works via delegation: the rootRakefileno longer loads the package Rakefile; it runs common tasks (lint,run_rspec:*,shakapacker_examples:*,node_package, etc.) insidereact_on_railswithbundle exec, auto-installing that bundle when needed. Rootrake lintis root-tooling RuboCop only; full package lint stayscd react_on_rails && bundle exec rake lint.Contributor and CI scripts (
AGENTS.md,bin/setup,bin/ci-local,bin/ci-rerun-failures,conductor-setup.sh) now install and invoke the correct Gemfile per directory via a sharedbundle_commandhelper. Package lint still invokes root RuboCop throughBUNDLE_GEMFILE=../GemfilewithBundler.with_unbundled_env.Release rake drops the
ReactOnRails::Utilsrequire and uses a localrelease_truthy?for dry-run/override flags.Reviewed by Cursor Bugbot for commit 8299358. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Chores
Documentation
Refactor
Agent Merge Confidence
Mode: accelerated-rc
Score: 9/10
Auto-merge recommendation: yes
Affected areas: root/OSS bundle separation, RuboCop lint routing, setup/CI scripts, Dependabot config.
Checks: current head
82993589292d0d9bd6e9fa0246eb22213b42f763has 49 passing checks and 5 expected selector-skipped checks; full CI and benchmark labels are present and exercised.Review threads: 0 unresolved current review threads.
Known residual risk: root bundle/package boundary touches setup and CI helper routing; mitigated by local shell/Rake/RuboCop/lint validations, full CI and benchmark pass, and current-head AI review checks.
Finalized by: Claude Code Review check
claude-reviewfor current head82993589292d0d9bd6e9fa0246eb22213b42f763(SUCCESS).