-
Notifications
You must be signed in to change notification settings - Fork 128
Upgrade to Rails 8.0 and introduce rate limit #890
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR upgrades the app from Rails 7.2 to Rails 8.0, refreshes default Rails error pages with modern HTML/CSS, and aligns environment configs and tooling with the new stack requirements.
- Bump Rails to 8.0.2 and
config.load_defaults 8.0
- Redesign all
public/*.html
error pages with up-to-date doctype, meta tags, and responsive CSS - Update environment configs (caching headers, SSL defaults, logging), recommend Node 22, and refine Dependabot settings
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
public/*.html | Replaced legacy Rails error pages with new responsive HTML/CSS pages |
config/application.rb | Updated config.load_defaults from 7.2 to 8.0 |
Gemfile | Upgraded Rails gem to ~> 8.0.2 |
README.md | Updated Node recommendation to 22 |
.github/dependabot.yml | Removed workaround ignore for sqlite3 |
config/environments/*.rb | Adjusted cache headers, SSL, logging tags, mailer URL options |
config/puma.rb | Added optional Solid Queue plugin loading |
config/initializers/filter_parameter_logging.rb | Added :cvv and :cvc to filtered parameters |
Comments suppressed due to low confidence (3)
public/404.html:7
- The status text ‘Not found’ should be capitalized as ‘Not Found’ for consistency and clarity in the title.
<title>The page you were looking for doesn’t exist (404 Not found)</title>
config/environments/development.rb:29
- By always setting
:memory_store
outside of thedev:cache
toggle, thetmp/caching-dev.txt
switch is ignored. You may want to restore:null_store
when caching is disabled to respect the Rails dev:cache behavior.
config.cache_store = :memory_store
config/environments/production.rb:35
- The
TaggedLogging.logger
call is not a valid constructor. UseActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(STDOUT))
or the Rails helper to properly wrap the logger for tagged logging.
config.logger = ActiveSupport::TaggedLogging.logger(STDOUT)
60c56cc
to
d5c9c69
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #890 +/- ##
==========================================
- Coverage 99.08% 99.05% -0.03%
==========================================
Files 14 14
Lines 109 106 -3
==========================================
- Hits 108 105 -3
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
assert_text "Admin user was successfully created." | ||
assert_text "[email protected]" | ||
assert_current_path admin_admin_user_path(AdminUser.last) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moving this below to give the server time to actually persist the new user
In fact, assert_current_path
may wait 2 seconds, but AdminUser.last
is set immediately, and there is no guarantee that the server did complete the creation in time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says:
moving this below to give the server time to actually persist the new user
In fact, assert_current_path may wait 2 seconds, but AdminUser.last is set immediately, and there is no guarantee that the server did complete the creation in time
Is this accurate?
Yes, the comment is accurate. Here’s why:
- In Rails system/integration tests, test code often interacts with both the client-side (browser actions, path assertions) and the server-side (database queries like AdminUser.last).
- assert_current_path may include a wait, but database queries such as AdminUser.last occur immediately in the test process and do not wait for asynchronous server-side actions.
- There is a real risk that the server has not finished persisting the new user by the time AdminUser.last is called, especially if there are asynchronous jobs, delayed DB commits, or the test is running in a threaded environment.
Moving the check (AdminUser.last) after an assertion or step that ensures the server has completed its work (such as a path assertion following a submit and redirect) helps ensure the user creation is actually complete before the test relies on it.
Summary:
The technical reasoning in the comment is correct and moving the code as described is a valid and often necessary practice in integration/system tests to avoid race conditions between client actions and server-side persistence.
3cfb72c
to
ff7e849
Compare
- Bump Rails to version 8.0 and execute `rails app:update` for configuration alignment - Recommend Node.js 22 for development and runtime compatibility - Enhance Dependabot configuration for improved dependency management - Remove suppression of SQLite production warning, addressing rails/rails#50463 - Use an example domain for email addresses in test suites - Reorder certain path assertions after text assertions in specs to ensure server state is persisted - Add the frozen string literal magic comment to Ruby files - Switch to Rails 8’s built-in rate limiter for request throttling Closes #637
ff7e849
to
a7a80fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Co-authored-by: Javier Julio <[email protected]>
I hope this will actually work on render |
Deploy succeeded, the app is running, but I'm not able to check if the rate limit works. It does not appear to do so |
rails app:update
forconfiguration alignment
Remove SQLite production warning but leave production config disabled rails/rails#50463
ensure server state is persisted
Close #637