Conversation
Coverage Report for CI Build 33420910360Coverage remained the same at 98.3%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
🟡 Changes recommended
The initializer’s ENV presence check treats an empty string as set, which can configure Rack::Attack with an invalid Redis URL and cause boot/runtime failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds support for using a dedicated Redis cache store for Rack::Attack throttle counters so that throttling activity doesn’t evict or interfere with the app’s primary result caching in production.
Changes:
- Introduces
RACK_ATTACK_REDIS_URLsupport to configure Rack::Attack with its ownRedisCacheStore. - Falls back to
Rails.cachewhenRACK_ATTACK_REDIS_URLis not set. - Documents the new environment variable in the README.
File summaries
| File | Description |
|---|---|
| README.md | Documents the new RACK_ATTACK_REDIS_URL environment variable and intended behavior. |
| config/initializers/rack_attack.rb | Adds conditional configuration to use a dedicated Redis cache store for Rack::Attack when configured. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The new Redis cache configuration disables TLS certificate verification (VERIFY_NONE), which is a security risk and should be changed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| Rack::Attack.cache.store = ActiveSupport::Cache::RedisCacheStore.new( | ||
| url: rack_attack_redis_url, | ||
| ssl_params: { verify_mode: OpenSSL::SSL::VERIFY_NONE } | ||
| ) |
There was a problem hiding this comment.
Nope, this is required.
https://help.heroku.com/HC0F8CUS/heroku-key-value-store-connection-issues
Why are these changes being introduced: * In production environments, we want to ensure that throttling does not purge our result caching (or vice versa). Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-656 How does this address that need: * Allows for using separate dedicated cache by checking ENV and using it if present. * In non-production ENV, it defaults back to simply using Rails.cache for simplicity.
Why are these changes being introduced:
Relevant ticket(s):
How does this address that need:
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing