Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughRemoves Coveralls integration and replaces it with SimpleCov across dev/test tooling, generators, Rake tasks, dummy binstubs, tests, and documentation; docs now require Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Greptile SummaryThis PR removes the stale Coveralls integration across both Confidence Score: 5/5Safe to merge — purely removes dead code and stale dependencies with no functional regressions. All changes are straightforward removals of an unmaintained integration. The only finding is a P2 description mismatch on the run_rspec:empty rake task, which does not affect runtime behaviour. react_on_rails_pro/rakelib/run_rspec.rake — minor task description mismatch after removing the automatic COVERAGE=true flag. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[rake default / rake run_rspec] --> B[run_rspec tasks]
B --> C[run_rspec:gem]
B --> D[run_rspec:dummy]
B --> E[run_rspec:empty]
B --> F[run_rspec:js_tests]
C --> G["bundle exec rspec (gem specs)"]
D --> H["bundle exec rspec (dummy app)"]
E --> I["rspec spec/empty_spec.rb\n(no COVERAGE=true)"]
F --> J["pnpm run test"]
K["COVERAGE=true\n(user-set)"] -.->|"opt-in"| G
K -.->|"opt-in"| H
style I fill:#fff3cd,stroke:#ffc107
Reviews (1): Last reviewed commit: "Use a working Slack URL in the README" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code Review — PR #3204: Remove stale Coveralls integrationOverviewClean, well-scoped removal of the unmaintained What works well
Issues / suggestions1. SimpleCov still locked to v0.16.1 in lock files
2. Slack community URL — invite link vs. workspace linkChanging 3.
|
![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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react_on_rails_pro/rakelib/run_rspec.rake`:
- Line 27: Replace the bare rspec invocation in the sh call so it uses the
Bundler environment; specifically update the command string "sh \"rspec
spec/empty_spec.rb\"" to use "sh \"bundle exec rspec spec/empty_spec.rb\"" so
the RSpec run in run_rspec.rake (the sh call that runs spec/empty_spec.rb) uses
the project's Gemfile and matches other invocations like run_tests_in.
In `@README.md`:
- Line 118: The README currently links "React + Rails Slack" to the workspace
URL that blocks new signups; replace the URL for that link with the public
invite URL provided
(https://join.slack.com/t/reactrails/shared_invite/zt-38oicm9d0-OO0V~bdg4aYNuZuUbRFSXg)
so the "React + Rails Slack" anchor allows new users to join and create accounts
during onboarding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4b044a0-7602-4a22-a034-82e6df3d16ec
⛔ Files ignored due to path filters (4)
react_on_rails/Gemfile.lockis excluded by!**/*.lockreact_on_rails/spec/dummy/Gemfile.lockis excluded by!**/*.lockreact_on_rails_pro/Gemfile.lockis excluded by!**/*.lockreact_on_rails_pro/spec/dummy/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
CONTRIBUTING.mdREADME.mdreact_on_rails/Gemfile.development_dependenciesreact_on_rails/Rakefilereact_on_rails/lib/generators/react_on_rails/dev_tests_generator.rbreact_on_rails/rakelib/run_rspec.rakereact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/spec/dummy/bin/coverallsreact_on_rails/spec/react_on_rails/generators/dev_tests_generator_spec.rbreact_on_rails_pro/CONTRIBUTING.mdreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/Rakefilereact_on_rails_pro/rakelib/run_rspec.rakereact_on_rails_pro/spec/dummy/bin/coveralls
💤 Files with no reviewable changes (5)
- react_on_rails_pro/spec/dummy/bin/coveralls
- react_on_rails_pro/Rakefile
- react_on_rails/Rakefile
- react_on_rails/rakelib/run_rspec.rake
- react_on_rails/spec/dummy/bin/coveralls
Code Review – Remove stale Coveralls integration (#3204)OverviewClean dependency-cleanup PR. Coveralls 0.8.23 was effectively abandoned and the whole integration was dead (badge pointed at an old Travis run). The PR correctly removes every Coveralls artifact – rake tasks, binstubs, gem entries – and registers a direct What works well
Issues / suggestions1. Slack URL may not be a self-join link 2. Trailing blank line after deleted content in 3. No version constraint on the new 4. One historical mention of Coveralls remains VerdictApprove with minor comments. The Slack URL is the only item worth a second look before merge; everything else is low-stakes. |
|
Follow-up on bot feedback:
If there is a newer public invite or a stable landing page for Slack onboarding, I can switch the docs to that in a follow-up. |
Code ReviewOverviewThis PR cleanly removes the stale Coveralls integration, replacing it with direct What's Good
Issues FoundBug: In gem("rspec-rails", group: :test)
gem("simplecov", require: false) # ← missing group: :testWhen the Minor: Slack workspace URL doesn't let new users join The PR replaces Minor: no version constraint on The old |
- Scope generated simplecov gem to the test group (matches the rspec-rails entry directly above and the dev Gemfiles) - Tighten the dev_tests generator spec to assert the new group - Clarify the Pro run_rspec:empty task description so it's obvious that COVERAGE=true is opt-in Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review triage summary (commit 80c86a8)I (Claude Code) walked the bot review feedback and split it into MUST-FIX, OPTIONAL, and SKIPPED. Code changes for the first two landed in 80c86a8; SKIPPED items got rationale replies on their threads and the resolved threads have been closed. Addressed in code (80c86a8)
Declined / non-actionable (replied with rationale, threads resolved)
Verification: |
Review: Remove stale Coveralls integrationOverall: Clean, low-risk cleanup — approve with two minor points. What this does well
Issues
Nits
|
- Pin `simplecov` to `~> 0.16.1` in both Gemfile.development_dependencies files so it follows the project's strict-version convention. Matches the version that was previously transitively pinned via coveralls and resolves consistently in all four lockfiles. - README: clarify that the React + Rails Slack workspace URL only works for existing members and direct non-members to GitHub Discussions. - run_rspec:empty: only invoke RSpec when COVERAGE=true; otherwise print a skip message. Without coverage enabled, the task would have loaded SimpleCov against an empty cache and produced a misleading report. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 2 review triage (commit 42d2e0c)I (Claude Code) walked the new claude[bot] feedback in #3204 (comment) and the two new inline comments. Code changes for the must-fix and the two optional refinements landed in 42d2e0c; the remaining nit is acknowledged below as a deliberate skip. Addressed in code (42d2e0c)
Deliberately skipped
Resolved both inline review threads. The |
Code Review: Remove stale Coveralls integrationOverviewClean, focused PR that removes the long-stale Coveralls integration across both OSS and Pro packages. The approach is correct — Coveralls was already pulling in SimpleCov as a transitive dependency, so switching to a direct SimpleCov dependency maintains identical behavior while eliminating the dead Coveralls layer. Low risk. Positive observations
Issues / observations1.
|
| gem "capybara", "~> 3.40" | ||
| gem "capybara-screenshot" | ||
| gem "coveralls", require: false | ||
| gem "simplecov", "~> 0.16.1", require: false |
There was a problem hiding this comment.
This version constraint (~> 0.16.1) is carried forward from the old coveralls gem requirement, which pinned to the same range. SimpleCov 0.16.x is from 2019; the current release line is 0.22.x, which adds branch coverage support and better HTML output. Consider loosening to ~> 0.22 either here or in a quick follow-up.
| gem "simplecov", "~> 0.16.1", require: false | |
| gem "simplecov", "~> 0.22", require: false |
|
|
||
| desc "run all tests" | ||
| task run_rspec: %i[gem dummy empty js_tests] do | ||
| puts "Completed all RSpec tests" |
There was a problem hiding this comment.
Good improvement — the task now skips cleanly instead of running a no-op spec. One edge case: spec/empty_spec.rb itself does an unconditional require "simplecov" (no COVERAGE guard), so if a developer runs rspec spec/empty_spec.rb directly without COVERAGE=true, SimpleCov will still be loaded and may emit output. The task-level guard here is correct, but adding a matching guard in spec/empty_spec.rb would make direct invocations consistent too.
1 participant
Summary
simplecovdependenciesCloses #1692.
Testing
Note
Low Risk
Low risk since changes are limited to test/CI tooling and documentation, but could affect coverage reporting if any workflows still expect Coveralls tasks or binstubs.
Overview
Removes the stale Coveralls integration across both OSS and Pro: drops the Coveralls badge, deletes Coveralls-only rake hooks/tasks, and removes Coveralls binstubs from dummy apps.
Switches coverage support to direct
simplecovdependencies (including thedev_testsgenerator and lockfiles) and updates docs to clarify that coverage reports are produced when running tests withCOVERAGE=true. Also updates the React+Rails Slack link to the direct workspace URL.Reviewed by Cursor Bugbot for commit 80c86a8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit