Upgrade to Rails 8.1 by jazairi · Pull Request #436 · MITLibraries/timdex-ui

jazairi · 2026-07-23T00:07:50Z

Developer

Accessibility

ANDI or WAVE has been run in accordance to our guide.
This PR contains no changes to the view layer.
New issues flagged by ANDI or WAVE have been resolved.
New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
No new accessibility issues have been flagged.

New ENV

All new ENV is documented in README.
All new ENV has been added to Heroku Pipeline, Staging and Prod.
ENV has not changed.

Approval beyond code review

UXWS/stakeholder approval has been confirmed.
UXWS/stakeholder review will be completed retroactively.
UXWS/stakeholder review is not needed.

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

I have confirmed that the code works as intended.
Any CodeClimate issues have been fixed or confirmed as
added technical debt.

Documentation

The commit message is clear and follows our guidelines
(not just this pull request message).
The documentation has been updated or is unnecessary.
New dependencies are appropriate or there were no changes.

Testing

There are appropriate tests covering any new functionality.
No additional test coverage is required.

coveralls · 2026-07-23T00:09:40Z

Coverage Report for CI Build 30031688818

Coverage remained the same at 98.384%

Details

Coverage remained the same as the base build.
Patch coverage: No coverable lines changed in this PR.
No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	1547
Covered Lines:	1522
Line Coverage:	98.38%
Coverage Strength:	85.47 hits per line

💛 - Coveralls

JPrevost

Overall looks good. I think we need to bring back our CSP and maybe reintroduce an option to Puma config.

Once we update those and land this, let's check GeoData in staging closely and then tag #proj_use to do some testing in unified search staging before we promote.

JPrevost · 2026-07-23T12:55:05Z

+# Rails.application.configure do
+#   config.content_security_policy do |policy|
+#     policy.default_src :self, :https
+#     policy.font_src    :self, :https, :data
+#     policy.img_src     :self, :https, :data
+#     policy.object_src  :none
+#     policy.script_src  :self, :https
+#     policy.style_src   :self, :https
+#     # Specify URI for violation reports
+#     # policy.report_uri "/csp-violation-report-endpoint"
+#   end


Please review this more closely. We had explicit settings in the old config we want to carry forward while updating the boilerplate. CSP is confusing (to me) and I don't remember why we had some of the settings, but they were required for...something?

Yeah, you're right. It was implemented here and enabled here. I'm not sure why it's needed either, but I agree a) that we should keep it in for now, and b) we should explore nonce-based CSPs.

JPrevost · 2026-07-23T12:58:37Z

-# Processes count, allows better CPU utilization when executing Ruby code.
-# Recommended to always run in at least one process so `rack-timeout` RACK_TERM_ON_TIMEOUT=1 can be used
-# https://devcenter.heroku.com/articles/h12-request-timeout-in-ruby-mri
-workers(ENV.fetch('WEB_CONCURRENCY') { 2 })


This one is confusing and I think comes from Heroku config. We set WEB_CONCURRENCY=1 in Heroku config and use rack-timeout (but don't seem to set RACK_TERM_ON_TIMEOUT=1 so 🤷🏻 ).

tldr; we may want to keep this around?

I'm not sure I agree. I think Heroku config should come from Heroku, not hardcoded in Puma config. Furthermore, a fallback of 2 feels oddly expensive to me. I think that if we want a setting of 2 in staging and prod, we should set that explicitly in our staging/prod apps.

To be fair, this is another instance of "I'm not sure why we did this," so the safest option is to keep it as-is. But, imo, the correct option is likely to modify our staging/prod apps to use the config we want.

Setting this in heroku config cannot affect anything unless we tell puma to read the value though. Heroku is telling us how to best run Puma in Heroku.

These tickets clarify somewhat why that setting is in place:

https://mitlibraries.atlassian.net/browse/USE-396

https://mitlibraries.atlassian.net/browse/TIMX-298 (yes, this is a ticket that no work was logged under but I'm pretty sure I just forgot there was a ticket when I did the work and noticed that recently so I closed it)

Also, the 2 value isn't how many dynos are running it's how many instances of the app run in each dyno. This allows us to most effectively use the RAM allocated in each dyno. Our dynos are so small we don't get many instances.

Ahh, gotcha. Thanks for clarifying.

JPrevost · 2026-07-23T13:06:06Z

  # Skip http-to-https redirect for the default health check endpoint.
  # config.ssl_options = { redirect: { exclude: ->(request) { request.path == "/up" } } }

-  # Prepend all log lines with the following tags.


Hmmm. I thought we'd need to keep this to get the request id but it seems maybe it is now default or semantic logger adds it. Nothing to do here, just talking out loud :)

Yup, semantic logger does this now.

jazairi · 2026-07-23T17:34:29Z

@JPrevost Let me know what you think about the Puma config. If we're aligned, I'll update staging/prod apps to confirm WEB_CONCURRENCY is set to 2, assuming that's the desired setting.

If you disagree, I don't mind leaving the config in the codebase. I'm not particularly firm in my opinion on this. I mostly don't understand why we're falling back to 2 here, other than to avoid updating a million staging/prod apps whenever we want to change it. (Which, honestly, is reason enough to leave it as-is.)

JPrevost

Approving and also totally open to change the fall back for WEB_CONCURRENCY to 1 based on what you said about it along the way.

JPrevost · 2026-07-23T18:17:19Z

 # Processes count, allows better CPU utilization when executing Ruby code.
 # Recommended to always run in at least one process so `rack-timeout` RACK_TERM_ON_TIMEOUT=1 can be used
 # https://devcenter.heroku.com/articles/h12-request-timeout-in-ruby-mri
 workers(ENV.fetch('WEB_CONCURRENCY') { 2 })


Based on our comments about this, I'd be pretty good with defaulting to 1 and then instances that want more can intentionally bump it up. What do you think about that?

If 2 is what is broadly needed, I trust that it's a reasonable fallback. It doesn't seem like it's causing issues thus far.

jazairi added 6 commits July 21, 2026 15:57

Bundle update Rails to 8.0

50a99ee

Load Rails 8.0 defaults

23ff7b2

Rails app:update 8.0 generated files

c54525a

Update renovate config to support Rails 8

f681a95

Bundle update to Rails 8.1 and load 8.1 defaults

e0279d0

Rails app:update 8.1 generated files

9b20598

jazairi requested review from JPrevost and matt-bernhardt July 23, 2026 00:10

mitlib temporarily deployed to timdex-ui-pi-rails8-55febtavuo July 23, 2026 00:10 Inactive

JPrevost requested changes Jul 23, 2026

View reviewed changes

Reintroduce CSP config

e181556

jazairi temporarily deployed to timdex-ui-pi-rails8-55febtavuo July 23, 2026 17:30 Inactive

jazairi requested a review from JPrevost July 23, 2026 17:34

Reintroduce workers config

b9ce8e3

jazairi temporarily deployed to timdex-ui-pi-rails8-55febtavuo July 23, 2026 17:58 Inactive

JPrevost approved these changes Jul 23, 2026

View reviewed changes

jazairi merged commit dcb737f into main Jul 23, 2026
6 checks passed

jazairi deleted the rails8 branch July 23, 2026 18:51

Uh oh!

Conversation

jazairi commented Jul 23, 2026

Developer

Accessibility

New ENV

Approval beyond code review

Additional context needed to review

Code Reviewer

Code

Documentation

Testing

Uh oh!

coveralls commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 30031688818

Coverage remained the same at 98.384%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

JPrevost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jazairi commented Jul 23, 2026

Uh oh!

JPrevost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jul 23, 2026 •

edited

Loading