Conversation
…ith tests ## Summary - Upgrade myapp from Rails 7.0 to Rails 8.1.3 with Phlex views and daisyUI components - Replace ERB/Slim views with Phlex components following patterns from daisyui/cosmos repos - Build a lock testing/debugging dashboard for sidekiq-unique-jobs - Add comprehensive test coverage for queue_and_lock Lua script (33 tests) - Add lock_sync! fast path tests to locksmith_spec (11 tests) - Add fast path integration tests across all lock types (38 tests) ## myapp Changes - Rails 7.0 -> 8.1, Tailwind 3 -> 4, sprockets -> propshaft - Added phlex-rails and daisyui gem for component-based views - New LocksController with dashboard showing all lock types - Phlex components: ApplicationLayout, Navbar, IndexView, ShowView - Removed Devise (unnecessary for test app), Slim, sinatra, cssbundling - Lock dashboard: enqueue jobs, view active locks, inspect lock state ## Test Coverage - spec/sidekiq_unique_jobs/lua/queue_and_lock_spec.rb: tests single-lock fast path, multi-lock path, TTL handling, all lock types, lock_score - spec/sidekiq_unique_jobs/locksmith_spec.rb: lock_sync! method tests covering sync_locked state, idempotency, lock_info, TTL, multi-limit - spec/sidekiq_unique_jobs/locksmith/fast_path_spec.rb: integration tests for all lock types via the combined queue_and_lock path
|
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:
📝 WalkthroughWalkthroughRails load defaults bumped to 8.1; Devise and its views/helpers/initializer removed; ERB layouts/partials replaced by Phlex components/layout; JS/CSS toolchain migrated to Bun + Tailwind v4 + DaisyUI; new Locks feature added to inspect/manage Sidekiq Unique Jobs (controller, Phlex views, routes); Sidekiq/lock-focused tests added. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Rails as Rails App
participant Sidekiq as Sidekiq
participant Redis as Redis
Browser->>Rails: GET /locks or GET /locks/:id
Rails->>Redis: read digests / digest keys
Redis-->>Rails: digests + lock metadata
Rails-->>Browser: render Phlex Locks::IndexView/ShowView
Browser->>Rails: POST /locks/enqueue (job_name,count)
Rails->>Sidekiq: enqueue perform_async(job_class, args) x N
Sidekiq->>Redis: push job + SidekiqUniqueJobs updates
Redis-->>Sidekiq: ACK
Rails-->>Browser: redirect with notice
Browser->>Rails: DELETE /locks/flush
Rails->>Redis: delete digests by pattern ("*")
Redis-->>Rails: deletion result
Rails-->>Browser: redirect with notice
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?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: 12
🧹 Nitpick comments (3)
spec/sidekiq_unique_jobs/locksmith/fast_path_spec.rb (1)
44-48: Consider clarifying the conditional expectation.The conditional
if lock_ttl.nil?on line 47 makes the test behavior context-dependent. Consider splitting this into separate examples or adding a comment explaining whyuntil_expiredlocks remain locked after unlock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/sidekiq_unique_jobs/locksmith/fast_path_spec.rb` around lines 44 - 48, The conditional expectation makes the test ambiguous; split into two clear examples (or add an explanatory comment) around locksmith_one.lock / locksmith_one.unlock: create one example for the case when lock_ttl is nil that asserts expect(locksmith_one).not_to be_locked, and another example for the "until_expired" case that asserts the lock remains locked after unlock (expect(locksmith_one).to be_locked) or add a short comment above the current expectation explaining why locks with lock_ttl (until_expired) remain locked after unlock; reference the locksmith_one.lock, locksmith_one.unlock, and lock_ttl/ until_expired symbols to locate the logic to change.spec/sidekiq_unique_jobs/lua/queue_and_lock_spec.rb (1)
262-274: Test title is misleading — only verifies combined path, not comparison.The test is titled "produces the same locked state as queue -> lmove -> lock" but only exercises the combined
queue_and_lockscript without actually testing the separate scripts for comparison. Consider either:
- Renaming to reflect what it actually tests (e.g., "correctly sets locked state")
- Adding the actual comparison with separate script calls
♻️ Proposed fix to clarify test intent
- context "when compared with separate queue+lock scripts" do - it "produces the same locked state as queue -> lmove -> lock" do - # Combined path - result_combined = call_script(:queue_and_lock, key.to_a, argv_one) - combined_locked = hget(key.locked, job_id_one) - combined_digest = get(key.digest) - - # Verify the combined script locked correctly - expect(result_combined).to eq(job_id_one) - expect(combined_locked).not_to be_nil - expect(combined_digest).to eq(job_id_one) - end - end + context "with combined script verification" do + it "correctly sets locked state" do + result = call_script(:queue_and_lock, key.to_a, argv_one) + + expect(result).to eq(job_id_one) + expect(hget(key.locked, job_id_one)).not_to be_nil + expect(get(key.digest)).to eq(job_id_one) + end + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/sidekiq_unique_jobs/lua/queue_and_lock_spec.rb` around lines 262 - 274, The test "produces the same locked state as queue -> lmove -> lock" only calls the combined script (queue_and_lock) and asserts its effects (result_combined, combined_locked via hget(key.locked, job_id_one), combined_digest via get(key.digest)), so either rename the example to reflect it only verifies the combined path (e.g., "correctly sets locked state") or extend the test to actually perform the separate steps (call_script for queue, then simulate lmove, then call_script for lock) and compare their resulting states to the combined run (compare the locked hash and digest values and the return value job_id_one). Make the change around the existing call_script(:queue_and_lock, key.to_a, argv_one) and assertions for result_combined, combined_locked, and combined_digest.myapp/app/controllers/locks_controller.rb (1)
101-104: Destructive action: consider adding confirmation or scoping.The
flushaction deletes ALL lock digests across the entire system using a wildcard pattern"*". While appropriate for a testing dashboard, consider:
- Adding a confirmation step in the UI
- Logging who triggered the flush for audit purposes
📝 Proposed enhancement to add logging
def flush + Rails.logger.info("Lock flush triggered from #{request.remote_ip}") SidekiqUniqueJobs::Digests.new.delete_by_pattern("*") redirect_to locks_path, notice: "All lock digests flushed" end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@myapp/app/controllers/locks_controller.rb` around lines 101 - 104, The flush action currently calls SidekiqUniqueJobs::Digests.new.delete_by_pattern("*") in the flush method which indiscriminately removes all digests; change this to require explicit confirmation and scope the deletion (e.g., accept a scoped pattern or specific lock IDs via params and validate a confirmation param) and record an audit entry (e.g., log current_user.id/email, timestamp and the pattern used) before performing deletions; update the flush controller method (flush) to check for confirmation/admin permission, derive a safe pattern from params (not raw "*"), call delete_by_pattern only with that safe pattern, and write an audit log entry via Rails.logger or an Audit model prior to redirect_to locks_path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@myapp/app/assets/stylesheets/application.tailwind.css`:
- Around line 4-21: Stylelint is flagging valid Tailwind v4 at-rules like
`@plugin`, `@source`, and `@theme` because the inherited at-rule-no-unknown rule from
stylelint-config-standard-scss doesn't recognize them; update your linter config
(the .stylelintrc.json rule "at-rule-no-unknown") to either set it to null or
configure it to ignore these at-rules (add "plugin", "source", "theme", and
optionally "import" to ignoreAtRules) so Tailwind directives pass validation.
In `@myapp/app/controllers/locks_controller.rb`:
- Line 4: Remove the unsafe CSRF bypass by deleting the skip_before_action
:verify_authenticity_token, only: [:enqueue, :flush] line and re-enable Rails'
CSRF protection for the enqueue and flush actions; ensure the frontend includes
and sends the Rails CSRF token for AJAX/Turbo requests so the controller methods
enqueue and flush continue to work normally.
- Around line 74-82: The enqueue action currently calls safe_constantize on
params[:job_name] without checking an allowlist; validate job_name against the
DEMO_JOBS allowlist first (e.g., ensure DEMO_JOBS.include?(job_name)) and
reject/redirect if not allowed, then call safe_constantize to obtain job_class;
update the conditional that handles unknown jobs to cover both not-in-allowlist
and safe_constantize returning nil so arbitrary classes in initialize are never
resolved for unapproved job names.
In `@myapp/app/jobs/issue_432_job.rb`:
- Around line 38-41: The log method currently builds the diagnostic suffix
string (using Time.now.to_i and redis.call('GET', 'start') / redis.call('GET',
'counter')) and returns it, but callers ignore the return value; change log to
Rails.logger.debug the timing/counter string instead of returning it (e.g.,
compose the suffix into a local variable and call Rails.logger.debug with it)
and then return nil or the original intended value so the diagnostic info is
actually written to logs; update the method named log to use Rails.logger.debug
for the suffix rather than relying on its return.
In `@myapp/app/views/locks/index_view.rb`:
- Around line 100-105: The enqueue_button helper renders unlabeled buttons;
update the enqueue_button method to provide an accessible name by adding an
aria-label (or hidden descriptive text) that includes both the job_name and
count (e.g. "Enqueue 3 jobs for <job_name>") so screen readers can distinguish
controls; modify the call that builds the button_to (the enqueue_button helper
referencing enqueue_locks_path) to include aria: { label: "..."} or insert
visually-hidden text inside the block, and handle singular/plural wording if
desired.
In `@myapp/config/application.rb`:
- Around line 28-30: The autoload entries for Phlex roots (config.autoload_paths
<< "#{root}/app/views", "#{root}/app/views/layouts",
"#{root}/app/views/components") must also be added to config.eager_load_paths so
view classes like IndexView, ShowView, ApplicationLayout, Navbar, etc. are
present when eager loading in production; update the same file to append those
three paths to config.eager_load_paths (using the same "#{root}/app/..."
strings) so they are eagerly loaded.
In `@myapp/config/initializers/assets.rb`:
- Around line 3-4: Update the misleading comment above the assets path addition:
replace the statement that Propshaft "automatically" serves assets with a clear
note that the path is being added manually because Propshaft does not include
app/assets/builds by default (we're adding Rails.application.config.assets.paths
<< Rails.root.join("app", "assets", "builds") manually, e.g., because
cssbundling-rails is not used and `@tailwindcss/cli` is used directly).
In `@myapp/config/routes.rb`:
- Around line 4-6: The Sidekiq::Web route is exposed without authentication;
wrap the Sidekiq::Web mount in basic HTTP auth to protect /sidekiq (modify the
mount in routes.rb where Sidekiq::Web is referenced). Implement
Rack::Auth::Basic (or a Rails routing constraint) that checks credentials
sourced from environment vars (e.g. SIDEKIQ_USERNAME and SIDEKIQ_PASSWORD) and
returns 401 on failure, ensuring the authentication is applied before calling
mount Sidekiq::Web so only authorized users can access the web UI.
In `@myapp/Gemfile`:
- Around line 7-26: Your Gemfile removed Devise but the User model still invokes
the Devise DSL (the call starting with "devise :database_authenticatable,
:registerable, ...") so the app will crash on boot; open the User model (class
User) and either re-add the devise gem to the Gemfile or remove the Devise DSL
invocation and any Devise-specific callbacks/concerns (e.g., the "devise" call
and related methods), and update/replace any Devise-dependent logic
(validations, scopes, modules) to use your new authentication approach so the
model no longer references Devise symbols.
In `@myapp/spec/requests/home_spec.rb`:
- Around line 5-33: Wrap the "Locks Dashboard" request spec group in a
SidekiqUniqueJobs.use_config block and set SidekiqUniqueJobs.config.enabled =
false for the duration of the group so global uniqueness and Redis state are
disabled during these examples; locate the RSpec.describe "Locks Dashboard"
block and modify it to run inside SidekiqUniqueJobs.use_config {
SidekiqUniqueJobs.config.enabled = false; ... } ensuring the config change is
scoped to the block (restored automatically) so the GET "/" and GET "/locks/:id"
examples run with uniqueness disabled.
In `@myapp/spec/system/static_pages_spec.rb`:
- Around line 5-9: Wrap the smoke spec so uniqueness is temporarily disabled
using SidekiqUniqueJobs.use_config and setting SidekiqUniqueJobs.config.enabled
= false for the duration of the test; locate the "Lock Testing Dashboard"
example (the RSpec.describe block and the it "can browse the dashboard" example)
and update the test body to run the visit "/" and expectation inside
SidekiqUniqueJobs.use_config { SidekiqUniqueJobs.config.enabled = false; ... }
so uniqueness state does not leak into this non-uniqueness spec.
In `@spec/sidekiq_unique_jobs/locksmith_spec.rb`:
- Around line 428-435: The test currently only checks the block ran but doesn't
fail if execute falls back to the synchronous path; update the spec to spy/stub
the sync-only method (lock_sync!) on the locksmith instance (locksmith_one)
before calling execute, then assert that lock_sync! was not called while still
asserting the block executed; reference the execute method and the lock_sync!
method on locksmith_one so the test will fail if execute regresses to the sync
path.
---
Nitpick comments:
In `@myapp/app/controllers/locks_controller.rb`:
- Around line 101-104: The flush action currently calls
SidekiqUniqueJobs::Digests.new.delete_by_pattern("*") in the flush method which
indiscriminately removes all digests; change this to require explicit
confirmation and scope the deletion (e.g., accept a scoped pattern or specific
lock IDs via params and validate a confirmation param) and record an audit entry
(e.g., log current_user.id/email, timestamp and the pattern used) before
performing deletions; update the flush controller method (flush) to check for
confirmation/admin permission, derive a safe pattern from params (not raw "*"),
call delete_by_pattern only with that safe pattern, and write an audit log entry
via Rails.logger or an Audit model prior to redirect_to locks_path.
In `@spec/sidekiq_unique_jobs/locksmith/fast_path_spec.rb`:
- Around line 44-48: The conditional expectation makes the test ambiguous; split
into two clear examples (or add an explanatory comment) around
locksmith_one.lock / locksmith_one.unlock: create one example for the case when
lock_ttl is nil that asserts expect(locksmith_one).not_to be_locked, and another
example for the "until_expired" case that asserts the lock remains locked after
unlock (expect(locksmith_one).to be_locked) or add a short comment above the
current expectation explaining why locks with lock_ttl (until_expired) remain
locked after unlock; reference the locksmith_one.lock, locksmith_one.unlock, and
lock_ttl/ until_expired symbols to locate the logic to change.
In `@spec/sidekiq_unique_jobs/lua/queue_and_lock_spec.rb`:
- Around line 262-274: The test "produces the same locked state as queue ->
lmove -> lock" only calls the combined script (queue_and_lock) and asserts its
effects (result_combined, combined_locked via hget(key.locked, job_id_one),
combined_digest via get(key.digest)), so either rename the example to reflect it
only verifies the combined path (e.g., "correctly sets locked state") or extend
the test to actually perform the separate steps (call_script for queue, then
simulate lmove, then call_script for lock) and compare their resulting states to
the combined run (compare the locked hash and digest values and the return value
job_id_one). Make the change around the existing call_script(:queue_and_lock,
key.to_a, argv_one) and assertions for result_combined, combined_locked, and
combined_digest.
🪄 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: d70b4ab3-db55-4ac0-b427-0b6d5f503982
⛔ Files ignored due to path filters (3)
myapp/bun.lockis excluded by!**/*.lockmyapp/package-lock.jsonis excluded by!**/package-lock.jsonmyapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (63)
myapp/.erb-lint.ymlmyapp/.eslintignoremyapp/.eslintrc.jsmyapp/Gemfilemyapp/Procfile.devmyapp/app/assets/stylesheets/application.tailwind.cssmyapp/app/controllers/application_controller.rbmyapp/app/controllers/home_controller.rbmyapp/app/controllers/issues_controller.rbmyapp/app/controllers/locks_controller.rbmyapp/app/helpers/devise_helper.rbmyapp/app/helpers/home_helper.rbmyapp/app/javascript/application.jsmyapp/app/javascript/controllers/application.jsmyapp/app/javascript/controllers/flash_controller.jsmyapp/app/javascript/controllers/index.jsmyapp/app/jobs/issue_432_job.rbmyapp/app/jobs/until_executing_job.rbmyapp/app/views/application/_header.html.erbmyapp/app/views/components/application_component.rbmyapp/app/views/components/application_view.rbmyapp/app/views/components/navbar.rbmyapp/app/views/devise/confirmations/new.html.erbmyapp/app/views/devise/mailer/confirmation_instructions.html.erbmyapp/app/views/devise/mailer/password_change.html.erbmyapp/app/views/devise/mailer/reset_password_instructions.html.erbmyapp/app/views/devise/mailer/unlock_instructions.html.erbmyapp/app/views/devise/passwords/edit.html.erbmyapp/app/views/devise/passwords/new.html.erbmyapp/app/views/devise/registrations/edit.html.erbmyapp/app/views/devise/registrations/new.html.erbmyapp/app/views/devise/sessions/new.html.erbmyapp/app/views/devise/shared/_form_footer.html.erbmyapp/app/views/devise/shared/_links.html.erbmyapp/app/views/devise/unlocks/new.html.erbmyapp/app/views/home/index.html.slimmyapp/app/views/issues/index.html.slimmyapp/app/views/layouts/application.html.erbmyapp/app/views/layouts/application_layout.rbmyapp/app/views/locks/index_view.rbmyapp/app/views/locks/show_view.rbmyapp/config/application.rbmyapp/config/environments/development.rbmyapp/config/environments/production.rbmyapp/config/environments/test.rbmyapp/config/importmap.rbmyapp/config/initializers/assets.rbmyapp/config/initializers/devise.rbmyapp/config/initializers/sidekiq.rbmyapp/config/routes.rbmyapp/db/schema.rbmyapp/package.jsonmyapp/postcss.config.jsmyapp/spec/factories/users.rbmyapp/spec/helpers/home_helper_spec.rbmyapp/spec/requests/home_spec.rbmyapp/spec/system/static_pages_spec.rbmyapp/spec/views/home/index.html.slim_spec.rbmyapp/stylelint.config.jsmyapp/tailwind.config.jsspec/sidekiq_unique_jobs/locksmith/fast_path_spec.rbspec/sidekiq_unique_jobs/locksmith_spec.rbspec/sidekiq_unique_jobs/lua/queue_and_lock_spec.rb
💤 Files with no reviewable changes (30)
- myapp/app/controllers/home_controller.rb
- myapp/spec/factories/users.rb
- myapp/app/views/devise/mailer/reset_password_instructions.html.erb
- myapp/app/helpers/home_helper.rb
- myapp/app/views/layouts/application.html.erb
- myapp/app/views/application/_header.html.erb
- myapp/app/views/devise/shared/_form_footer.html.erb
- myapp/app/views/devise/mailer/confirmation_instructions.html.erb
- myapp/app/views/devise/mailer/password_change.html.erb
- myapp/spec/views/home/index.html.slim_spec.rb
- myapp/app/views/devise/registrations/new.html.erb
- myapp/app/views/home/index.html.slim
- myapp/app/views/devise/mailer/unlock_instructions.html.erb
- myapp/spec/helpers/home_helper_spec.rb
- myapp/.erb-lint.yml
- myapp/postcss.config.js
- myapp/.eslintignore
- myapp/app/views/devise/passwords/edit.html.erb
- myapp/app/views/devise/passwords/new.html.erb
- myapp/app/views/devise/confirmations/new.html.erb
- myapp/app/views/devise/unlocks/new.html.erb
- myapp/app/views/devise/sessions/new.html.erb
- myapp/tailwind.config.js
- myapp/.eslintrc.js
- myapp/app/views/devise/shared/_links.html.erb
- myapp/app/controllers/issues_controller.rb
- myapp/stylelint.config.js
- myapp/app/helpers/devise_helper.rb
- myapp/config/initializers/devise.rb
- myapp/app/views/devise/registrations/edit.html.erb
- Validate job_name against DEMO_JOBS allowlist before constantize (RCE fix) - Remove Devise DSL from User model (gem was removed from Gemfile) - Fix Issue432Job log method to include diagnostic info in log output - Add aria-label to enqueue buttons for accessibility - Fix misleading comment in assets initializer - Improve locksmith execute test to verify async path via spy - Upgrade to Ruby 4.0.2, Bun 1.3.11, Bundler 4.0.9 - Add .ruby-version, .bun-version, update .tool-versions
There was a problem hiding this comment.
🧹 Nitpick comments (2)
myapp/app/jobs/issue_432_job.rb (1)
34-36: Consider using a connection pool for RedisClient.Each job instance creates its own
RedisClientconnection. While acceptable for this dev/test app, in a production context you'd typically useRedisClient.config(...).new_poolor leverage Sidekiq's existing Redis connection pool to avoid connection churn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@myapp/app/jobs/issue_432_job.rb` around lines 34 - 36, The redis method currently builds a new RedisClient per job instance (`@redis` ||= RedisClient.new(...)), which can create many connections; change it to use a shared connection pool (e.g., call RedisClient.config(url: ENV.fetch("REDIS_URL", nil)).new_pool or use Sidekiq's existing pool) and update callers in Issue432Job to check out a connection from that pool (use pool.with or equivalent) instead of referencing `@redis` directly; ensure the pool is memoized (e.g., `@redis_pool`) and created once for the process.myapp/app/controllers/locks_controller.rb (1)
74-99: Static analysis RCE warning is a false positive; optional defense-in-depth improvement available.The Brakeman/CodeQL warnings about
constantizeon line 83 are false positives because the allowlist check (DEMO_JOBS.key?(job_name)) at lines 78-81 ensures only the 9 predefined job names can reachconstantize.As a minor defense-in-depth measure, consider using
safe_constantizeto gracefully handle the edge case where a job class inDEMO_JOBSdoesn't exist at runtime (e.g., file deleted but constant not removed):🛡️ Optional: use safe_constantize for defense-in-depth
- job_class = job_name.constantize + job_class = job_name.safe_constantize + unless job_class + redirect_to locks_path, alert: "Job class not found: #{job_name}" + return + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@myapp/app/controllers/locks_controller.rb` around lines 74 - 99, The enqueue action currently calls job_name.constantize after checking DEMO_JOBS, which is sufficient but can fail if the constant is unexpectedly missing; change job_name.constantize to job_name.safe_constantize and handle a nil job_class (e.g., redirect_to locks_path with an alert like "Job class not loaded" or treat as failed enqueue) so enqueue gracefully handles the rare runtime-missing-class case; reference the enqueue method, DEMO_JOBS allowlist, and the job_class variable when making this defensive change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@myapp/app/controllers/locks_controller.rb`:
- Around line 74-99: The enqueue action currently calls job_name.constantize
after checking DEMO_JOBS, which is sufficient but can fail if the constant is
unexpectedly missing; change job_name.constantize to job_name.safe_constantize
and handle a nil job_class (e.g., redirect_to locks_path with an alert like "Job
class not loaded" or treat as failed enqueue) so enqueue gracefully handles the
rare runtime-missing-class case; reference the enqueue method, DEMO_JOBS
allowlist, and the job_class variable when making this defensive change.
In `@myapp/app/jobs/issue_432_job.rb`:
- Around line 34-36: The redis method currently builds a new RedisClient per job
instance (`@redis` ||= RedisClient.new(...)), which can create many connections;
change it to use a shared connection pool (e.g., call RedisClient.config(url:
ENV.fetch("REDIS_URL", nil)).new_pool or use Sidekiq's existing pool) and update
callers in Issue432Job to check out a connection from that pool (use pool.with
or equivalent) instead of referencing `@redis` directly; ensure the pool is
memoized (e.g., `@redis_pool`) and created once for the process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bcaf4412-2cdb-4e05-9a12-94c6a8764b2b
⛔ Files ignored due to path filters (1)
myapp/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
myapp/.bun-versionmyapp/.ruby-versionmyapp/.tool-versionsmyapp/Gemfilemyapp/app/controllers/locks_controller.rbmyapp/app/jobs/issue_432_job.rbmyapp/app/models/user.rbmyapp/app/views/locks/index_view.rbmyapp/config/initializers/assets.rbmyapp/db/schema.rbmyapp/package.jsonspec/sidekiq_unique_jobs/locksmith_spec.rb
💤 Files with no reviewable changes (1)
- myapp/app/models/user.rb
✅ Files skipped from review due to trivial changes (4)
- myapp/.ruby-version
- myapp/.bun-version
- myapp/.tool-versions
- myapp/app/views/locks/index_view.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- myapp/config/initializers/assets.rb
- myapp/Gemfile
- spec/sidekiq_unique_jobs/locksmith_spec.rb
- myapp/db/schema.rb
The seeds file referenced Devise methods (password, confirm) that no longer exist after removing the gem. The lock testing dashboard doesn't need any database records.
overmind is not installed. Use the same process manager fallback chain as cosmos: hivemind -> foreman -> install foreman.
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 `@myapp/bin/dev`:
- Line 5: The Procfile.dev is unsetting the PORT environment so the export in
bin/dev (and overrides like PORT=4000 bin/dev) never reach Puma; edit
Procfile.dev to remove the "unset PORT" from the command that currently runs
"unset PORT && bin/rails server" so the process uses the exported PORT, ensuring
commands like PORT=4000 bin/dev and the bin/dev export take effect for Puma and
the rails server.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- Switch to dracula theme for dark mode look - Color-coded lock type badges per lock strategy - Hero section header with centered layout - Individual stat cards with SVG icons instead of stats bar - Cards with icons, dividers, hover effects, and mockup-code for args - Empty state illustrations for no-locks - Sticky navbar with lock icon and mobile dropdown - Footer showing gem/Rails/Ruby versions - Improved show view with grid config display and collapsible JSON
- Add rails_icons gem with heroicons library for proper SVG icons - Replace all inline SVG paths with hero() helper calls - Create IconHelper module included in ApplicationComponent - Fix SystemStackError in enqueue action - rescue infinite recursion caused by on_conflict: :reschedule re-enqueueing immediately - Show rejected job count in flash message
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
myapp/app/views/locks/show_view.rb (1)
87-105: Theconstantizecall is safe here given the controller's whitelist.The controller validates
job_nameagainstDEMO_JOBSbefore passing it to this view (seelocks_controller.rb:62-72), so only known job class names can reach this code path. Since this is a localhost-only dev tool, this is acceptable.Consider adding a brief comment for future maintainers:
def job_config_card div(class: "card bg-base-100 shadow-sm") do div(class: "card-body") do h2(class: "card-title text-lg mb-2") { "Sidekiq Options" } + # Safe: job_name validated against DEMO_JOBS whitelist in controller job_class = `@job_name.constantize`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@myapp/app/views/locks/show_view.rb` around lines 87 - 105, Add a short inline comment above the job_config_card's use of `@job_name.constantize` explaining that this constantize is safe because LocksController validates job_name against the DEMO_JOBS whitelist (see LocksController#... where job_name is checked) and that this view is only used in a localhost/dev-only tool; reference the job_config_card method, the `@job_name` variable, and the DEMO_JOBS whitelist so future maintainers understand the safety rationale.myapp/app/views/locks/index_view.rb (2)
53-65: Remove redundant button text argument.When
button_tois given both a name argument and a block, the block content is rendered and the name is ignored. Line 53's"Flush All Locks"is dead code since the block (lines 57-64) provides the button content.♻️ Suggested fix
- button_to "Flush All Locks", flush_locks_path, + button_to flush_locks_path, method: :delete, class: "btn btn-error btn-outline btn-sm gap-2", data: { turbo_confirm: "This will delete all lock digests. Continue?" } do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@myapp/app/views/locks/index_view.rb` around lines 53 - 65, The button_to call is passing a name argument ("Flush All Locks") which is ignored when a block is provided; remove the redundant first argument so the call becomes button_to flush_locks_path, method: :delete, class: "btn btn-error btn-outline btn-sm gap-2", data: { turbo_confirm: "This will delete all lock digests. Continue?" } do ... end and keep the existing block content (the svg and plain "Flush All Locks") as the rendered button content; update the invocation around the button_to call in the view to eliminate the dead string argument.
25-29: Consider makingqueue_statsexplicitly optional.The
view_templatechecksif@queue_stats`` (line 34), implying it can be nil. Making the parameter explicitly optional improves API clarity:- def initialize(jobs:, digests:, queue_stats:) + def initialize(jobs:, digests:, queue_stats: nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@myapp/app/views/locks/index_view.rb` around lines 25 - 29, The initialize method currently requires queue_stats but the template checks if `@queue_stats`, so make queue_stats explicitly optional by changing the initialize signature to accept queue_stats: nil and keep assigning `@queue_stats` = queue_stats; update any callers that pass queue_stats (or rely on default nil) and add a short comment on initialize or param docs to reflect that queue_stats may be nil; ensure view_template continues to guard with if `@queue_stats`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@myapp/app/views/locks/show_view.rb`:
- Around line 189-194: The dynamic class interpolation in state_stat (the
"text-#{color}" usage) can be stripped by Tailwind JIT; replace it with a
deterministic lookup map that maps permitted color keys (e.g., "info",
"warning", "success", "neutral") to concrete class strings and use that lookup
when building the div class, or alternatively add those classes to the Tailwind
safelist in tailwind.config.js; update the state_stat method to read from the
lookup (e.g., color_class = COLOR_MAP[color.to_s] || 'text-neutral') and use
that variable instead of interpolating to ensure classes are present in
production.
---
Nitpick comments:
In `@myapp/app/views/locks/index_view.rb`:
- Around line 53-65: The button_to call is passing a name argument ("Flush All
Locks") which is ignored when a block is provided; remove the redundant first
argument so the call becomes button_to flush_locks_path, method: :delete, class:
"btn btn-error btn-outline btn-sm gap-2", data: { turbo_confirm: "This will
delete all lock digests. Continue?" } do ... end and keep the existing block
content (the svg and plain "Flush All Locks") as the rendered button content;
update the invocation around the button_to call in the view to eliminate the
dead string argument.
- Around line 25-29: The initialize method currently requires queue_stats but
the template checks if `@queue_stats`, so make queue_stats explicitly optional by
changing the initialize signature to accept queue_stats: nil and keep assigning
`@queue_stats` = queue_stats; update any callers that pass queue_stats (or rely on
default nil) and add a short comment on initialize or param docs to reflect that
queue_stats may be nil; ensure view_template continues to guard with if
`@queue_stats`.
In `@myapp/app/views/locks/show_view.rb`:
- Around line 87-105: Add a short inline comment above the job_config_card's use
of `@job_name.constantize` explaining that this constantize is safe because
LocksController validates job_name against the DEMO_JOBS whitelist (see
LocksController#... where job_name is checked) and that this view is only used
in a localhost/dev-only tool; reference the job_config_card method, the
`@job_name` variable, and the DEMO_JOBS whitelist so future maintainers understand
the safety rationale.
🪄 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: 88d7f951-737b-4e8b-810b-40774fef4dbc
📒 Files selected for processing (5)
myapp/app/views/components/footer.rbmyapp/app/views/components/navbar.rbmyapp/app/views/layouts/application_layout.rbmyapp/app/views/locks/index_view.rbmyapp/app/views/locks/show_view.rb
✅ Files skipped from review due to trivial changes (1)
- myapp/app/views/components/footer.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- myapp/app/views/layouts/application_layout.rb
- myapp/app/views/components/navbar.rb
Dynamic "text-#{color}" gets purged by Tailwind's JIT compiler.
Use STAT_COLORS constant with complete class names.
- Use plugins syntax instead of require (rubocop 1.86+) - Enforce double_quotes for string literals - Add rubocop-thread_safety plugin (inherited from parent config) - Remove myapp/**/* exclusion from parent .rubocop.yml - Set SuggestExtensions: false
Remove rubocop-thread_safety from both parent gem and myapp. Update myapp .rubocop.yml with inherit_from: [] to prevent parent config inheritance, enforce double_quotes for string literals.
…ting! Sidekiq 8 deprecated `require "sidekiq/testing"` in favor of the explicit `Sidekiq.testing!(:fake)` API. Updated the gem's own testing.rb module which is the canonical entry point.
This was referenced Mar 28, 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.
Summary
daisyuiandcosmosreference projectsqueue_and_lockLua script andlock_sync!fast pathmyapp Upgrade
Lock Testing Dashboard
Test Coverage for Performance Optimizations
queue_and_lock_spec.rb(33 tests): single-lock fast path, multi-lock path, re-lock idempotency, limit enforcement, TTL/PEXPIRE, all lock types, score handlinglocksmith_spec.rbadditions (11 tests):lock_sync!sync_locked state, lock_info, TTL, multi-limitfast_path_spec.rb(38 tests): integration tests for fast path across all lock types (until_executed, until_executing, until_expired, while_executing, until_and_while_executing), blocking vs sync path verificationTest plan
bundle exec rspec spec/sidekiq_unique_jobs/lua/queue_and_lock_spec.rb— 33 passbundle exec rspec spec/sidekiq_unique_jobs/locksmith_spec.rb— 37 passbundle exec rspec spec/sidekiq_unique_jobs/locksmith/fast_path_spec.rb— 38 passbundle exec rubocop— no offenses on new filesSummary by CodeRabbit
New Features
Refactor
Tests