Open
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 selected for processing (1)
📝 WalkthroughWalkthroughUpgrade Ruby to 4.0.1 and Rails to 8.1; bump many gems and adapt Elasticsearch to v8; add GIT_TAG build arg and use it for runtime VERSION; introduce CI scripts and security checks; adjust Docker base image, initializers, environment configs, and several bin scripts. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/jobs/event_index_job_spec.rb (1)
86-106:⚠️ Potential issue | 🟡 MinorTest uses
SocketErrorinstead ofElastic::Transport::Transport::Error.The describe block names
Elastic::Transport::Transport::Error, but line 87 creates aSocketErrorinstance. This test doesn't actually verify that the job rescues fromElastic::Transport::Transport::Error— it passes coincidentally becauseSocketErroris also in the rescue list.🔧 Proposed fix to use the correct exception class
describe "Elastic::Transport::Transport::Error" do - let(:error) { SocketError.allocate } + let(:error) { Elastic::Transport::Transport::Error.allocate }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/jobs/event_index_job_spec.rb` around lines 86 - 106, The test is instantiating SocketError instead of the intended Elastic::Transport::Transport::Error; change the let(:error) to create an instance of Elastic::Transport::Transport::Error (e.g., construct or allocate that class) and keep the existing stubbing of error.message and the allow(...).to(receive(:index_document).and_raise(error)) so the job actually raises and rescues the Elastic::Transport::Transport::Error when described_class.perform_now(event) is called; update only the error creation (referenced by let(:error)) so the rest of the spec (event.__elasticsearch__, index_document, described_class.perform_now) stays the same.
🧹 Nitpick comments (3)
config/initializers/new_framework_defaults_8_1.rb (1)
5-7: This staged-upgrade initializer is now misleading.
config/application.rbLine 22 already opts the app into 8.1 defaults, so this file reads like the 8.1 switches are still pending when the global flip has already happened. Either delete it, or keep the app on the olderload_defaultstarget until you actually roll these settings out one by one. (api.rubyonrails.org)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/initializers/new_framework_defaults_8_1.rb` around lines 5 - 7, The staged-upgrade initializer new_framework_defaults_8_1.rb is misleading because the application is already opting into Rails 8.1 via config.load_defaults in your application configuration; either remove this initializer to avoid confusion or revert the global opt-in by changing the config.load_defaults call (the Application.config.load_defaults setting) back to the previous target so you can truly roll the individual switches in new_framework_defaults_8_1.rb one-by-one.bin/dev (1)
2-2: Anchor thebin/railspath to the script directory.
exec "./bin/rails"depends on the caller's current working directory, so this wrapper breaks ifbin/devis invoked outside the repo root. Use__dir__here to make it location-independent.♻️ Suggested change
-exec "./bin/rails", "server", *ARGV +exec File.expand_path("rails", __dir__), "server", *ARGV🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/dev` at line 2, The exec call currently uses a relative path ("exec \"./bin/rails\", \"server\", *ARGV") which breaks when bin/dev is run outside the repo root; change the exec invocation to use an anchored path built from __dir__ so it always resolves to the script's directory (i.e., construct the absolute path to bin/rails using __dir__ and pass that to exec in place of "./bin/rails") and keep the remaining arguments ("server", *ARGV) unchanged.bin/bundler-audit (1)
5-5: Don't append a second--configwhen one was supplied explicitly.With the current guard,
bin/bundler-audit check --config other.ymlends up with two config flags, so callers can't cleanly override the default config.♻️ Suggested change
-ARGV.concat %w[ --config config/bundler-audit.yml ] if ARGV.empty? || ARGV.include?("check") +if (ARGV.empty? || ARGV.include?("check")) && !ARGV.include?("--config") + ARGV.concat %w[--config config/bundler-audit.yml] +end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/bundler-audit` at line 5, The current ARGV.concat line appends the default --config even when the caller already passed --config, producing duplicate flags; change the condition so the default is only appended when ARGV is empty or contains just "check" and no existing --config argument. Update the guard around ARGV.concat %w[ --config config/bundler-audit.yml ] to also verify no element in ARGV starts_with?("--config") (or equals "--config"), e.g., only concat when (ARGV.empty? || ARGV == ["check"]) && ARGV.none? { |a| a.start_with?("--config") }, so calls like bin/bundler-audit check --config other.yml do not get a second --config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/parallel_ci.yml:
- Around line 36-39: Replace the direct invocation of bundler-audit with the
repo wrapper so the project's ignore list is respected: update the Security
Check step to call the wrapper (bin/bundler-audit) instead of running "bundle
exec bundler-audit check --update" so config/bundler-audit.yml is applied; keep
the rest of the step (and brakeman call) unchanged.
In `@config/application.rb`:
- Line 22: The RuboCop offense is caused by an unparenthesized method call to
config.load_defaults; update the call to use parentheses (i.e., change
config.load_defaults 8.1 to config.load_defaults(8.1)) so the
Style/MethodCallWithArgsParentheses rule is satisfied and CI will pass; edit the
config.load_defaults usage in application.rb accordingly.
In `@config/bundler-audit.yml`:
- Around line 1-5: The CI workflow is invoking bundler-audit directly (bundle
exec bundler-audit check --update) so it bypasses the wrapper that appends
config/bundler-audit.yml; update the parallel_ci.yml job to run the wrapper
(bin/bundler-audit check --update) instead of calling bundler-audit directly, or
alternatively modify the wrapper script referenced by bin/bundler-audit to be
invoked by the workflow; ensure the command change causes the ignore list in
config/bundler-audit.yml to be applied during CI.
In `@config/environments/development.rb`:
- Around line 20-24: The dev:cache toggle only sets public_file_server.headers
but never enables Action Controller caching; update the if branch to set
config.action_controller.perform_caching = true (and typically also set a dev
cache store, e.g. config.cache_store = :memory_store) alongside
config.public_file_server.headers so that tmp/caching-dev.txt actually turns on
controller/fragment caching; leave the else branch as-is where
config.action_controller.perform_caching = false.
In `@config/initializers/_version.rb`:
- Line 3: The line defining the VERSION constant has incorrect indentation width
(1 space) causing a RuboCop offense; update the indentation to use 2 spaces
before the assignment so the file follows the project's 2-space indentation rule
(ensure the line with VERSION = ENV.fetch("GIT_TAG", "1.0.0") and any
surrounding lines in this initializer use two spaces consistently).
In `@config/initializers/filter_parameter_logging.rb`:
- Around line 6-8: The array assigned to
Rails.application.config.filter_parameters is missing the trailing comma on the
last element, triggering Style/TrailingCommaInArrayLiteral; update the array
literal used in the filter_parameters assignment (the
Rails.application.config.filter_parameters array) to include a trailing comma
after the final symbol so the linter stops failing.
In `@config/initializers/opensearch.rb`:
- Around line 8-15: The module ElasticsearchV8OpenSearchBypass has RuboCop
indentation offenses; reformat so Ruby style is followed: indent the private
keyword and the verify_elasticsearch method two spaces inside the module, with
the method body lines (response = `@transport.perform_request`(*args, &block),
`@verified` = true, response) indented two additional spaces, and ensure the
closing end keywords line up with their openings; locate the module name
ElasticsearchV8OpenSearchBypass and the verify_elasticsearch method to apply
these changes.
In `@Dockerfile`:
- Around line 3-9: The Dockerfile sets ARG GIT_TAG and ENV GIT_TAG early which
makes ENV GIT_TAG a cache invalidation point and forces expensive layers
(apt-get, bundle install) to rebuild; move the ARG GIT_TAG=1.0 and the ENV
GIT_TAG=${GIT_TAG} block to the end of the Dockerfile after all expensive build
steps so GIT_TAG is only applied at runtime, ensuring earlier layers keep their
cache; specifically remove or relocate the existing ARG GIT_TAG/ENV GIT_TAG near
the top and add them as a final block "ARG GIT_TAG=1.0" followed by "ENV
GIT_TAG=${GIT_TAG}" after the bundle install and other heavy RUN instructions.
---
Outside diff comments:
In `@spec/jobs/event_index_job_spec.rb`:
- Around line 86-106: The test is instantiating SocketError instead of the
intended Elastic::Transport::Transport::Error; change the let(:error) to create
an instance of Elastic::Transport::Transport::Error (e.g., construct or allocate
that class) and keep the existing stubbing of error.message and the
allow(...).to(receive(:index_document).and_raise(error)) so the job actually
raises and rescues the Elastic::Transport::Transport::Error when
described_class.perform_now(event) is called; update only the error creation
(referenced by let(:error)) so the rest of the spec (event.__elasticsearch__,
index_document, described_class.perform_now) stays the same.
---
Nitpick comments:
In `@bin/bundler-audit`:
- Line 5: The current ARGV.concat line appends the default --config even when
the caller already passed --config, producing duplicate flags; change the
condition so the default is only appended when ARGV is empty or contains just
"check" and no existing --config argument. Update the guard around ARGV.concat
%w[ --config config/bundler-audit.yml ] to also verify no element in ARGV
starts_with?("--config") (or equals "--config"), e.g., only concat when
(ARGV.empty? || ARGV == ["check"]) && ARGV.none? { |a| a.start_with?("--config")
}, so calls like bin/bundler-audit check --config other.yml do not get a second
--config.
In `@bin/dev`:
- Line 2: The exec call currently uses a relative path ("exec \"./bin/rails\",
\"server\", *ARGV") which breaks when bin/dev is run outside the repo root;
change the exec invocation to use an anchored path built from __dir__ so it
always resolves to the script's directory (i.e., construct the absolute path to
bin/rails using __dir__ and pass that to exec in place of "./bin/rails") and
keep the remaining arguments ("server", *ARGV) unchanged.
In `@config/initializers/new_framework_defaults_8_1.rb`:
- Around line 5-7: The staged-upgrade initializer new_framework_defaults_8_1.rb
is misleading because the application is already opting into Rails 8.1 via
config.load_defaults in your application configuration; either remove this
initializer to avoid confusion or revert the global opt-in by changing the
config.load_defaults call (the Application.config.load_defaults setting) back to
the previous target so you can truly roll the individual switches in
new_framework_defaults_8_1.rb one-by-one.
🪄 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: 2c6bfae9-3f2a-4313-89e8-80b108dda4b6
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/build.yml.github/workflows/parallel_ci.yml.github/workflows/rubocop.yml.ruby-versionDockerfileGemfileapp/jobs/event_index_job.rbbin/bundler-auditbin/cibin/devbin/rubocopbin/setupconfig/application.rbconfig/bundler-audit.ymlconfig/ci.rbconfig/environments/development.rbconfig/environments/test.rbconfig/initializers/_version.rbconfig/initializers/filter_parameter_logging.rbconfig/initializers/new_framework_defaults_7_2.rbconfig/initializers/new_framework_defaults_8_1.rbconfig/initializers/opensearch.rbspec/jobs/event_index_job_spec.rbspec/rails_helper.rb
💤 Files with no reviewable changes (1)
- config/initializers/new_framework_defaults_7_2.rb
Comment on lines
+1
to
+5
| # Audit all gems listed in the Gemfile for known security problems by running bin/bundler-audit. | ||
| # CVEs that are not relevant to the application can be enumerated on the ignore list below. | ||
|
|
||
| ignore: | ||
| - CVE-THAT-DOES-NOT-APPLY |
There was a problem hiding this comment.
This config is skipped by the GitHub Actions path.
The ignore list is only appended when callers go through bin/bundler-audit, but .github/workflows/parallel_ci.yml invokes bundle exec bundler-audit check --update directly. That means this file is silently ignored in that workflow, and any CVEs listed here won't actually be suppressed there.
💡 Align the workflow with the configured wrapper
- bundle exec bundler-audit check --update
+ bin/bundler-audit check --update🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/bundler-audit.yml` around lines 1 - 5, The CI workflow is invoking
bundler-audit directly (bundle exec bundler-audit check --update) so it bypasses
the wrapper that appends config/bundler-audit.yml; update the parallel_ci.yml
job to run the wrapper (bin/bundler-audit check --update) instead of calling
bundler-audit directly, or alternatively modify the wrapper script referenced by
bin/bundler-audit to be invoked by the workflow; ensure the command change
causes the ignore list in config/bundler-audit.yml to be applied during CI.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/initializers/_version.rb`:
- Line 3: Replace the unconditional default fallback when setting VERSION so
deploys fail fast in stage/production: change the assignment that currently uses
VERSION = ENV.fetch("GIT_TAG", "1.0.0") to conditionally require GIT_TAG in
production/stage (e.g., if Rails.env.production? || Rails.env.staging? then
VERSION = ENV.fetch("GIT_TAG") else VERSION = ENV.fetch("GIT_TAG", "1.0.0") ),
so missing GIT_TAG raises in stage/prod but preserves the local/dev fallback.
🪄 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: ebffb128-63c8-4ade-92e2-7584b0e4af37
📒 Files selected for processing (5)
.rubocop.ymlconfig/application.rbconfig/initializers/_version.rbconfig/initializers/filter_parameter_logging.rbconfig/initializers/opensearch.rb
💤 Files with no reviewable changes (1)
- .rubocop.yml
✅ Files skipped from review due to trivial changes (1)
- config/initializers/filter_parameter_logging.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- config/application.rb
- config/initializers/opensearch.rb
kaysiz
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Gemfile (1)
14-14: Movedotenvto development and test groups.Currently,
dotenvis loaded as a production dependency, but it should only be used in development and test environments. The code inconfig/application.rbalready guards the require with a file existence check, making this change safe—if no.envfile exists in production, dotenv is never required.♻️ Proposed change
-gem "dotenv", "~> 3.2" group :development, :test do + gem "dotenv", "~> 3.2" gem "debug", "~> 1.11", ">= 1.11.1", platforms: [:mri, :windows]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gemfile` at line 14, The Gemfile currently lists the dotenv gem as a general dependency; move gem "dotenv", "~> 3.2" into a development and test group so it’s only loaded in those environments—wrap it in group :development, :test do ... end (remove the top-level declaration) and then run bundle install to update the lockfile; this change targets the dotenv entry in the Gemfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Gemfile`:
- Line 14: The Gemfile currently lists the dotenv gem as a general dependency;
move gem "dotenv", "~> 3.2" into a development and test group so it’s only
loaded in those environments—wrap it in group :development, :test do ... end
(remove the top-level declaration) and then run bundle install to update the
lockfile; this change targets the dotenv entry in the Gemfile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d7a0761-dc69-4ed0-baf9-b639d8f46208
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
Gemfile
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
jrhoads
approved these changes
Apr 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
closes: Add github issue that originated this PR
Approach
Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines:
Summary by CodeRabbit
Chores
Bug Fixes