Conversation
| if options[:authenticate_routes_with_devise] | ||
| readme "README" | ||
| else | ||
| say "\nWe've added Sidekiq routes. Please protect it as necessary to suit your requirements.", |
There was a problem hiding this comment.
Let's update to " ... ... Please protect it with authentication as necessary ... ... ..."
|
I just remembered, we also need to add the command to run the sidekiq to Procfile.dev (or Procfile in general, research) if it exists so it runs when we do "bin/dev" |
rails auto generate Procfile.dev and it seems Procfile is used mostly for the heroku, should I check for both file? |
|
@TheZero0-ctrl We should only support Procfile.dev generated by rails in the beginning, user should have capability to skip this configuration as well. |
| aliases: "-ar", | ||
| default: false, | ||
| desc: "Tell us if you want sidekiq routes to only be accessed by authenticated users. Defaults to false." | ||
| class_option :skip_adding_sidekiq_to_procfile, |
There was a problem hiding this comment.
Let's update to skip_procfile_config / _configuration whichever you want.
| say "Adding sidekiq worker to Procfile.dev", :green | ||
| append_to_file "Procfile.dev", "worker: bundle exec sidekiq" | ||
| else | ||
| say "Procfile.dev not found. Skipping adding sidekiq worker to Procfile.dev", :yellow |
There was a problem hiding this comment.
It's fine to return early if File is not found, we don't need to show file not found message
TheZero0-ctrl
force-pushed
the
generator-for-sidekiq
branch
4 times, most recently
from
cd73ee5 to
c9abf62
Compare
TheZero0-ctrl
force-pushed
the
generator-for-sidekiq
branch
from
c9abf62 to
9f91d3f
Compare
coolprobn
added
the
PR CREATED
TheZero0-ctrl
force-pushed
the
generator-for-sidekiq
branch
2 times, most recently
from
beaea6b to
c5622a5
Compare
TheZero0-ctrl
force-pushed
the
generator-for-sidekiq
branch
from
c5622a5 to
0a4fe67
Compare
WalkthroughAdds a new Rails generator to install and configure Sidekiq, updates documentation (CHANGELOG, README), provides a template README note for authenticated routes, and introduces tests covering generator behaviors and options. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Rails as Rails Generator CLI
participant Gen as Boring::Sidekiq::InstallGenerator
participant Bundler as Bundler
participant App as config/application.rb
participant Routes as config/routes.rb
participant Proc as Procfile.dev
Dev->>Rails: rails generate boring:sidekiq:install [options]
Rails->>Gen: invoke
Gen->>Bundler: add "sidekiq" gem + bundle install
Gen->>App: set config.active_job.queue_adapter = :sidekiq
alt Web UI not skipped
alt Authenticated not skipped
Gen->>Routes: mount Sidekiq::Web within authenticate :user
else Authenticated skipped
Gen->>Routes: mount Sidekiq::Web without auth
end
else Web UI skipped
Gen->>Routes: no changes to Sidekiq::Web
end
alt Procfile config not skipped and Procfile.dev exists
Gen->>Proc: append worker: bundle exec sidekiq
else Skipped or missing
Gen->>Proc: no change
end
Gen-->>Dev: output next steps / README note
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 5
🧹 Nitpick comments (5)
README.md (1)
101-101: Document flags for the new generator.Add a short note showing available options (
--skip_web_ui,--skip_authenticated_web_ui,--skip_procfile_config) so users don’t have to discover them via--help.Apply this diff to extend the bullet:
-- Install Sidekiq: `rails generate boring:sidekiq:install` +- Install Sidekiq: `rails generate boring:sidekiq:install` (flags: `--skip_web_ui`, `--skip_authenticated_web_ui`, `--skip_procfile_config`)lib/generators/boring/sidekiq/install/templates/README (1)
1-12: Clarify Devise dependency and alternatives.Explicitly state that the snippet requires Devise and suggest alternatives (basic auth or custom constraints) when Devise isn’t present.
Apply this diff to add a one‑liner:
We've protected Sidekiq routes with Devise to ensure only authenticated users can access them. This is implemented in the following snippet located in the config/routes.rb file. +Note: This requires Devise. If you don't use Devise, protect the route via another mechanism (e.g., HTTP basic auth or a custom constraint).test/generators/sidekiq_install_generator_test.rb (1)
18-36: Add idempotency checks and newline handling for Procfile.dev.
- Run the generator twice and assert no duplicate route or adapter lines.
- Ensure
worker:is appended on a new line even if Procfile.dev lacks a trailing newline.I can send a follow-up test patch if you want it bundled here.
lib/generators/boring/sidekiq/install/install_generator.rb (2)
38-47: Make adapter injection idempotent.Re-running the generator will duplicate
config.active_job.queue_adapterinconfig/application.rb.Apply this diff:
def set_sidekiq_as_active_job_adapter say "Setting sidekiq as active_job adapter", :green - - inject_into_file "config/application.rb", - optimize_indentation( - "config.active_job.queue_adapter = :sidekiq\n", - 4 - ), - after: /class Application < Rails::Application\n/ + path = "config/application.rb" + content = File.read(path) if File.exist?(path) + return if content&.include?("config.active_job.queue_adapter = :sidekiq") + + inject_into_file path, + optimize_indentation("config.active_job.queue_adapter = :sidekiq\n", 4), + after: /class Application < Rails::Application\n/ end
65-72: Guard against duplicate route injections.Re-running the generator will duplicate both
requireandmountentries.Apply this diff:
- inject_into_file "config/routes.rb", - "require 'sidekiq/web'\\n\\n", - before: "Rails.application.routes.draw do\\n" + routes_path = "config/routes.rb" + routes_content = File.read(routes_path) + unless routes_content.include?("require 'sidekiq/web'") + inject_into_file routes_path, "require 'sidekiq/web'\\n\\n", + before: "Rails.application.routes.draw do\\n" + end - inject_into_file "config/routes.rb", - optimize_indentation(route, 2), - after: "Rails.application.routes.draw do\\n" + mount_signature = "mount Sidekiq::Web => '/sidekiq'" + unless routes_content.include?(mount_signature) + inject_into_file routes_path, optimize_indentation(route, 2), + after: "Rails.application.routes.draw do\\n" + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)README.md(1 hunks)lib/generators/boring/sidekiq/install/install_generator.rb(1 hunks)lib/generators/boring/sidekiq/install/templates/README(1 hunks)test/generators/sidekiq_install_generator_test.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/generators/boring/sidekiq/install/install_generator.rb (1)
lib/boring_generators/generator_helper.rb (2)
check_and_install_gem(22-32)bundle_install(18-20)
test/generators/sidekiq_install_generator_test.rb (3)
lib/generators/boring/sidekiq/install/install_generator.rb (1)
include(7-91)test/generator_helper.rb (1)
app_path(15-22)test/test_helper.rb (1)
assert_gem(13-19)
🔇 Additional comments (1)
CHANGELOG.md (1)
5-5: Changelog entry reads well.Entry is clear and placed under master (unreleased). No action needed.
| def add_sidekiq_gem | ||
| say "Adding sidekiq gem to Gemfile", :green | ||
| check_and_install_gem("sidekiq") | ||
| bundle_install | ||
| end |
There was a problem hiding this comment.
Avoid double bundling; check_and_install_gem already runs bundle install.
This currently triggers bundle install twice, slowing the generator.
Apply this diff:
def add_sidekiq_gem
say "Adding sidekiq gem to Gemfile", :green
check_and_install_gem("sidekiq")
- bundle_install
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def add_sidekiq_gem | |
| say "Adding sidekiq gem to Gemfile", :green | |
| check_and_install_gem("sidekiq") | |
| bundle_install | |
| end | |
| def add_sidekiq_gem | |
| say "Adding sidekiq gem to Gemfile", :green | |
| check_and_install_gem("sidekiq") | |
| end |
🤖 Prompt for AI Agents
In lib/generators/boring/sidekiq/install/install_generator.rb around lines 32 to
36, the generator calls bundle_install after check_and_install_gem even though
check_and_install_gem already performs bundling; remove the redundant
bundle_install call so the Gemfile is only bundled once, leaving the say message
and check_and_install_gem invocation intact.
| def add_sidekiq_web_ui | ||
| return if options[:skip_web_ui] | ||
|
|
||
| say "Adding sidekiq web UI", :green | ||
|
|
||
| if options[:skip_authenticated_web_ui] | ||
| route = "mount Sidekiq::Web => '/sidekiq'\n\n" | ||
| else | ||
| route = <<~RUBY | ||
| authenticate :user do | ||
| mount Sidekiq::Web => '/sidekiq' | ||
| end | ||
|
|
||
| RUBY | ||
| end | ||
|
|
There was a problem hiding this comment.
Don’t assume Devise; generate a safe route that works without it.
Using authenticate :user will break apps without Devise. Provide a conditional fallback so routes compile even when Devise isn’t installed.
Apply this diff:
def add_sidekiq_web_ui
return if options[:skip_web_ui]
say "Adding sidekiq web UI", :green
- if options[:skip_authenticated_web_ui]
- route = "mount Sidekiq::Web => '/sidekiq'\n\n"
- else
- route = <<~RUBY
- authenticate :user do
- mount Sidekiq::Web => '/sidekiq'
- end
-
- RUBY
- end
+ mount_line = "mount Sidekiq::Web => '/sidekiq'"
+ if options[:skip_authenticated_web_ui]
+ route = "#{mount_line}\n\n"
+ else
+ route = <<~RUBY
+ if defined?(Devise)
+ authenticate :user do
+ #{mount_line}
+ end
+ else
+ #{mount_line}
+ end
+
+ RUBY
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def add_sidekiq_web_ui | |
| return if options[:skip_web_ui] | |
| say "Adding sidekiq web UI", :green | |
| if options[:skip_authenticated_web_ui] | |
| route = "mount Sidekiq::Web => '/sidekiq'\n\n" | |
| else | |
| route = <<~RUBY | |
| authenticate :user do | |
| mount Sidekiq::Web => '/sidekiq' | |
| end | |
| RUBY | |
| end | |
| def add_sidekiq_web_ui | |
| return if options[:skip_web_ui] | |
| say "Adding sidekiq web UI", :green | |
| mount_line = "mount Sidekiq::Web => '/sidekiq'" | |
| if options[:skip_authenticated_web_ui] | |
| route = "#{mount_line}\n\n" | |
| else | |
| route = <<~RUBY | |
| if defined?(Devise) | |
| authenticate :user do | |
| #{mount_line} | |
| end | |
| else | |
| #{mount_line} | |
| end | |
| RUBY | |
| end |
🤖 Prompt for AI Agents
In lib/generators/boring/sidekiq/install/install_generator.rb around lines 49 to
64, the generator unconditionally uses authenticate :user (Devise), which will
break apps without Devise; change the generated route to wrap the authenticate
block in a runtime check and provide a fallback mount so routes compile without
Devise. Replace the current route assignment with a HEREDOC that emits: if
defined?(Devise) then authenticate :user do mount Sidekiq::Web => '/sidekiq' end
else mount Sidekiq::Web => '/sidekiq' end (preserving newlines/indentation) so
the generated routes work whether or not Devise is installed.
| def add_sidekiq_worker_to_procfile | ||
| return if options[:skip_procfile_config] || !File.exist?("Procfile.dev") | ||
|
|
||
| say "Adding sidekiq worker to Procfile.dev", :green | ||
| append_to_file "Procfile.dev", "worker: bundle exec sidekiq" | ||
| end |
There was a problem hiding this comment.
Append worker: on a new line and avoid duplicates in Procfile.dev.
Without a newline, the worker entry can be glued to the last line. Also, prevent multiple worker lines across repeated runs.
Apply this diff:
def add_sidekiq_worker_to_procfile
return if options[:skip_procfile_config] || !File.exist?("Procfile.dev")
say "Adding sidekiq worker to Procfile.dev", :green
- append_to_file "Procfile.dev", "worker: bundle exec sidekiq"
+ path = "Procfile.dev"
+ content = File.read(path)
+ return if content.include?("worker: bundle exec sidekiq")
+
+ needs_newline = !content.end_with?("\n")
+ append_to_file path, "#{needs_newline ? "\n" : ""}worker: bundle exec sidekiq\n"
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def add_sidekiq_worker_to_procfile | |
| return if options[:skip_procfile_config] || !File.exist?("Procfile.dev") | |
| say "Adding sidekiq worker to Procfile.dev", :green | |
| append_to_file "Procfile.dev", "worker: bundle exec sidekiq" | |
| end | |
| def add_sidekiq_worker_to_procfile | |
| return if options[:skip_procfile_config] || !File.exist?("Procfile.dev") | |
| say "Adding sidekiq worker to Procfile.dev", :green | |
| path = "Procfile.dev" | |
| content = File.read(path) | |
| return if content.include?("worker: bundle exec sidekiq") | |
| needs_newline = !content.end_with?("\n") | |
| append_to_file path, "#{needs_newline ? "\n" : ""}worker: bundle exec sidekiq\n" | |
| end |
🤖 Prompt for AI Agents
In lib/generators/boring/sidekiq/install/install_generator.rb around lines 74 to
79, the generator appends "worker: bundle exec sidekiq" without ensuring a
preceding newline and without checking for duplicates; update the method to
first detect an existing Procfile.dev entry for a sidekiq worker and do nothing
if found, otherwise append the worker line preceded by a newline (or ensure the
file ends with a newline before appending) so the new entry stays on its own
line and repeated runs do not create duplicate worker lines.
| def show_message | ||
| return if options[:skip_routes] | ||
|
|
||
| if options[:authenticate_routes_with_devise] | ||
| readme "README" | ||
| else | ||
| say "\nWe've added Sidekiq routes. Please protect it as necessary to suit your requirements.", | ||
| :yellow | ||
| end | ||
| end |
There was a problem hiding this comment.
Fix options mismatch in show_message.
skip_routes and authenticate_routes_with_devise aren’t declared; the message may be misleading when --skip_web_ui is used. Use existing options to decide what to print/read.
Apply this diff:
def show_message
- return if options[:skip_routes]
-
- if options[:authenticate_routes_with_devise]
- readme "README"
- else
- say "\nWe've added Sidekiq routes. Please protect it as necessary to suit your requirements.",
- :yellow
- end
+ return if options[:skip_web_ui]
+
+ if options[:skip_authenticated_web_ui]
+ say "\nWe've added Sidekiq routes. Please protect it with authentication as necessary to suit your requirements.",
+ :yellow
+ else
+ readme "README"
+ end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def show_message | |
| return if options[:skip_routes] | |
| if options[:authenticate_routes_with_devise] | |
| readme "README" | |
| else | |
| say "\nWe've added Sidekiq routes. Please protect it as necessary to suit your requirements.", | |
| :yellow | |
| end | |
| end | |
| def show_message | |
| return if options[:skip_web_ui] | |
| if options[:skip_authenticated_web_ui] | |
| say "\nWe've added Sidekiq routes. Please protect it with authentication as necessary to suit your requirements.", | |
| :yellow | |
| else | |
| readme "README" | |
| end | |
| end |
🤖 Prompt for AI Agents
In lib/generators/boring/sidekiq/install/install_generator.rb around lines 81 to
90, the show_message method checks non-existent options (:skip_routes and
:authenticate_routes_with_devise) which can produce incorrect output; change it
to use the actual options—return early if options[:skip_web_ui], and branch on
options[:authenticate_with_devise] so that when authenticate_with_devise is true
you call readme "README" otherwise you print the yellow notice about protecting
the Sidekiq web UI.
| def test_should_skip_sidekiq_web_ui | ||
| Dir.chdir(app_path) do | ||
| quietly { run_generator %w[--skip_web_ui] } | ||
|
|
||
| assert_file "config/routes.rb" do |content| | ||
| assert_no_match(/mount Sidekiq::Web/, content) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick
Also assert require 'sidekiq/web' is not added when Web UI is skipped.
Strengthens the contract when --skip_web_ui is used.
Apply this diff:
assert_file "config/routes.rb" do |content|
assert_no_match(/mount Sidekiq::Web/, content)
+ assert_no_match(/require 'sidekiq\/web'/, content)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_should_skip_sidekiq_web_ui | |
| Dir.chdir(app_path) do | |
| quietly { run_generator %w[--skip_web_ui] } | |
| assert_file "config/routes.rb" do |content| | |
| assert_no_match(/mount Sidekiq::Web/, content) | |
| end | |
| end | |
| end | |
| def test_should_skip_sidekiq_web_ui | |
| Dir.chdir(app_path) do | |
| quietly { run_generator %w[--skip_web_ui] } | |
| assert_file "config/routes.rb" do |content| | |
| assert_no_match(/mount Sidekiq::Web/, content) | |
| assert_no_match(/require 'sidekiq\/web'/, content) | |
| end | |
| end | |
| end |
🤖 Prompt for AI Agents
In test/generators/sidekiq_install_generator_test.rb around lines 39 to 47, the
test for --skip_web_ui checks that routes do not mount Sidekiq::Web but doesn't
assert that the require 'sidekiq/web' line is also omitted; update the assertion
block to also assert_no_match for a require statement (e.g.
assert_no_match(/require ['"]sidekiq\/web['"]/, content)) so the test fails if
the generator inadvertently adds the require when Web UI is skipped.
2 participants
Summary by CodeRabbit