ReactionView is Cranky About Internal Rails Templates

[jherdman]

Greetings!

It seems like ReactionView doesn't like templates internal to Rails itself:

Error:
AscentsTest#test_user_can_delete_their_ascent_from_the_profile_page:
ActionView::Template::Error: 
HTML+ERB Compilation Errors:
============================================================

Error #1: InvalidNestingError
----------------------------------------
  File: /Users/jamesherdman/.local/share/mise/installs/ruby/4.0.1/lib/ruby/gems/4.0.0/gems/actionpack-8.1.2.1/lib/action_dispatch/middleware/templates/rescues/routing_error.html.erb
  Location: Line 9, Column 6
  Message: Block element <h2> cannot be nested inside <p> at line 9

  Source:
     6 |   <h2><%= h @exception_wrapper.message %></h2>
     7 |   <% unless @exception_wrapper.failures.empty? %>
     8 |     <p>
     9 |       <h2>Failure reasons:</h2>
              ^~~~~~~~~~~~~~~~~~~~~~~~~
    10 |       <ol>
    11 |       <% @exception_wrapper.failures.each do |route, reason| %>
    12 |         <li><code><%= route.inspect.delete('\\') %></code> failed because <%= reason.downcase %></li>

This happens when I run my Rails system tests with this configuration using ReactionView v0.3:

ReActionView.configure do |config|
  # Intercept .html.erb templates and process them with `Herb::Engine` for enhanced features
  config.intercept_erb = true # << KEY LINE IS HERE

  # Enable debug mode in development (adds debug attributes to HTML)
  config.debug_mode = Rails.env.development?

  # Validation mode (:raise, :overlay, or :none) — defaults to :raise in test, :overlay otherwise
  # config.validation_mode = :overlay

  # Add custom transform visitors to process templates before compilation
  # config.transform_visitors = [
  #   Herb::Visitor::new
  # ]
end

The workaround is simply not to have ReactionView intercept ERb.

Ought ReactionView not intercept HTML for gems?

As an aside, this template is fixed in upstream Rails rails/rails@84023df

[marcoroth]

Hey @jherdman, thanks for reporting!

Interesting, I thought we addressed this with #74, #56, and #45.

We ended up with this here, which I don't think should be matchting for Rails internal templates 🤔

reactionview/lib/reactionview/template/handlers/herb.rb

Lines 34 to 44 in 416995f

	def layout_template?(template)
	return false unless template.respond_to?(:identifier) && template.identifier
	template.identifier.include?("/layouts/")
	end
	def local_template?(template)
	return true unless template.respond_to?(:identifier) && template.identifier
	template.identifier.start_with?(Rails.root.to_s)
	end

Are you running on Rails main (via git) and/or use vendor/bundle/? I guess in those cases it could technically still start_with?(Rails.root) 🤔

[jherdman]

Neither Rails main, nor vendor/bundle. Is it just that we need an external_template? function that looks at the file path for template (see above error message)?

[jherdman]

OK, this makes sense to me but I'm a little unsure how to proceed. Here's the gist of how I understand this bug:

1. We start processing an external (i.e. not from my app) template at /Users/jamesherdman/.local/share/mise/installs/ruby/4.0.1/lib/ruby/gems/4.0.0/gems/actionpack-8.1.3/lib/action_dispatch/middleware/templates/rescues/routing_error.html.erb
2. We correctly avoid trying to inject the devtools in the code you looked at above, HOWEVER... we still try and process the template on L29 of the same file.

L29 errors out per the invalid HTML document.

IMHO the correct source of action is to ignore documents that don't belong to the app.

I reckon the correct course of action is to log a warning and fallback to ERB proper.

[marcoroth]

@jherdman, oh I think you are right 🤔

The idea was that we skip processing templates outside of your app with Herb::Engine, but we either lost this code in the back and forth in the mentioned PRs or we actually didn't end up implementing it 🤔

Either way, I think we shouldn't try to process templates that are out of your control using Herb. Or, maybe even better, try using Herb, and if it fails to parse surface the errors in a non-obtrusive way so that you can either try to fix the template upstream or ignore them somehow. In that case, we should fallback to the ERB engine that is actually registered for the case we are in the config.intercept_erb = true mode.

Personally I'd prefer this way, so we can surface these issues and improve the Herb Parser as a result.

Thanks for the investigation! 🙏🏼

[jherdman]

I'm very interested in carrying this issue to completion. Are you okay with me taking a little time to propose a solution in the form of a PR?

[marcoroth]

@jherdman definitely, that would be very much welcome, thank you! 🙏🏼

[jherdman]

Should be resolved by #94