Skip to content

Conversation

@tagliala
Copy link
Contributor

@tagliala tagliala commented Oct 22, 2025

Update ActiveAdmin routing code to use keyword arguments
(**options_hash) for methods such as namespace and get, addressing
deprecation warnings introduced in Rails 8.1 and ensuring compatibility
with Rails 8.2+. Previously, hash options were passed as positional
arguments, which will be unsupported in future Rails releases.

This change affects route definition logic in ActiveAdmin::Router,
including namespace and custom page actions. All routing methods now
use keyword arguments for options, improving code clarity and
future-proofing against Rails upgrades.

Ref: rails/rails#53689

@tagliala
Copy link
Contributor Author

Failures:

  1) Routing route_options with a custom path set in route_options should route using the custom path
     Failure/Error:
       router.namespace namespace.name, namespace.route_options.dup do
         router.root namespace.root_to_options.merge(to: namespace.root_to, as: :root)
       end
     
     ActiveSupport::DeprecationException:
       DEPRECATION WARNING: namespace received a hash argument path. Please use a keyword instead. Support to hash argument will be removed in Rails 8.2. (called from block in ActiveAdmin::Router#define_root_routes at /Users/geremia/dev/activeadmin/lib/active_admin/router.rb:24)

@tagliala
Copy link
Contributor Author

@tagliala tagliala requested a review from Copilot October 22, 2025 15:15
Copy link
Contributor

Copilot AI left a 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 updates the project to test against Rails 8.1.0 while maintaining Rails 8.0.0 compatibility. The changes modernize routing syntax to align with Rails 8.1 conventions and add the new rails_81 test configuration.

  • Updates Rails version from 8.0.0 to 8.1.0 in main Gemfile and test templates
  • Modernizes routing syntax with keyword arguments and forwarding operators
  • Adds rails_80 as a separate test matrix configuration with dependency management

Reviewed Changes

Copilot reviewed 7 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tasks/test_application.rb Updates app name and adds skip-bundler-audit flag for Rails 8.1
tasks/bug_report_template.rb Updates Rails version in bug report template to 8.1.0
lib/active_admin/router.rb Modernizes routing syntax with keyword arguments and argument forwarding
gemfiles/rails_80/Gemfile Adds new Gemfile for testing Rails 8.0.0 compatibility
Gemfile Updates main Rails version to 8.1.0
.github/workflows/ci.yaml Adds rails_81 to test matrix and updates conditional logic
.github/dependabot.yml Adds dependency management configuration for rails_80 Gemfile

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@codecov
Copy link

codecov bot commented Oct 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.08%. Comparing base (213360c) to head (39d955a).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8837   +/-   ##
=======================================
  Coverage   99.08%   99.08%           
=======================================
  Files         139      139           
  Lines        4046     4046           
=======================================
  Hits         4009     4009           
  Misses         37       37           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

router.root namespace.root_to_options.merge(to: namespace.root_to)
else
router.namespace namespace.name, namespace.route_options.dup do
router.namespace namespace.name, **namespace.route_options.dup do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

production change.

dup may still be needed to avoid nested objects in route options

def page_routes(config)
page = config.underscored_resource_name
router.get "/#{page}" => "#{page}#index"
router.get "/#{page}", to: "#{page}#index"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

production change.


def define_namespace(config)
router.namespace config.namespace.name, config.namespace.route_options.dup do
router.namespace config.namespace.name, **config.namespace.route_options.dup do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

production change.

dup may still be needed to avoid nested objects in route options

Comment on lines +94 to +95
def build_route(verbs, ...)
Array.wrap(verbs).each { |verb| router.send(verb, ...) }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

production change. this will allow to forward both Strings and Hashes

@tagliala tagliala marked this pull request as ready for review October 22, 2025 15:25
--skip-active-storage
--skip-bootsnap
--skip-brakeman
--skip-bundler-audit
Copy link
Contributor Author

@tagliala tagliala Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new flag

Update ActiveAdmin routing code to use keyword arguments
(`**options_hash`) for methods such as `namespace` and `get`, addressing
deprecation warnings introduced in Rails 8.1 and ensuring compatibility
with Rails 8.2+. Previously, hash options were passed as positional
arguments, which will be unsupported in future Rails releases.

This change affects route definition logic in `ActiveAdmin::Router`,
including namespace and custom page actions. All routing methods now
use keyword arguments for options, improving code clarity and
future-proofing against Rails upgrades.

Ref: rails/rails#53689
@tagliala tagliala changed the title Test against Rails 8.1.0 Add Rails 8.1.0 compatibility Oct 23, 2025
Copy link
Contributor

@mgrunberg mgrunberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I normally wouldn't include this in the changelog but I think it's worthwhile due to the routing changes even though it should be transparent to users.

@tagliala tagliala merged commit 542d796 into master Oct 23, 2025
31 checks passed
@tagliala tagliala deleted the feature/rails-81 branch October 23, 2025 15:24
@tagliala
Copy link
Contributor Author

Should this be backported to 3-stable ?

@mgrunberg
Copy link
Contributor

Should this be backported to 3-stable ?

I would love to have it in v3. Do you have time to do the backport? If not I can work on that but next week.

@tagliala
Copy link
Contributor Author

3-0-stable requires Ruby >= 2.6

s.required_ruby_version = ">= 2.6"

I guess that this will not allow (...)

@mgrunberg
Copy link
Contributor

3-0-stable requires Ruby >= 2.6

s.required_ruby_version = ">= 2.6"

I guess that this will not allow (...)

I think that operator got introduced in ruby 2.7, right?

I asked @javierjulio this morning and it should be ok to drop ruby 2.6 support in v3 if that blocks Rails 8.1 usage.

We also talked about using conditional code based on ruby version but it may be hard to read in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants