dejan / rails_panel

Chrome extension for Rails development
https://chromewebstore.google.com/detail/railspanel/gjpfobpafnhjhbajcjgccbbdofdckggg
MIT License
3.85k stars 187 forks source link

Improve Rails 7.2 compatibility #207

Closed jamiecobbett closed 2 weeks ago

jamiecobbett commented 1 month ago

On trying to upgrade to Rails 7.2, I was experiencing

[SystemStackError: stack level too deep (SystemStackError)]

On debugging, I found the object triggering this was an instance of ActiveRecord::Transaction:

=> #<ActiveRecord::Transaction:0x00000001211be2a8
 @internal_transaction=
  #<ActiveRecord::ConnectionAdapters::RealTransaction:0x0000000121b1a298
   @callbacks=nil,
   @connection=#<ActiveRecord::ConnectionAdapters::PostgreSQLAdapter:0x0000000000ec40 env_name="development" role=:writing>,
   @dirty=false,
   @instrumenter=
    #<ActiveRecord::ConnectionAdapters::TransactionInstrumenter:0x00000001211be258
     @base_payload=
      {:connection=>#<ActiveRecord::ConnectionAdapters::PostgreSQLAdapter:0x0000000000ec40 env_name="development" role=:writing>,
       :transaction=>#<ActiveRecord::Transaction:0x00000001211be2a8 ...>},
     @handle=nil,
     @payload=nil,
     @started=false>,
   @isolation_level=nil,
   @joinable=true,
   @lazy_enrollment_records=nil,
   @materialized=false,
   @records=
    [#<User REDACTED>],
   @run_commit_callbacks=true,
   @state=#<ActiveRecord::ConnectionAdapters::TransactionState:0x00000001211be348 @children=nil, @state=nil>,
   @user_transaction=#<ActiveRecord::Transaction:0x00000001211be2a8 ...>,
   @written=true>,
 @uuid=nil>

Adding this condition in appears to fix the problem.

I think this fixes https://github.com/dejan/rails_panel/issues/206

This kind of problem seems to be a perennial issue in the gem. I know that super_diff has similar issues - they have a "RecursionGuard", maybe that pattern could be applied to meta_request too?

jamiecobbett commented 3 weeks ago

@dejan let me know if there's anything I can do to make merging this easier - we are so grateful that this gem and extension exist ❤️

mahmoudimus commented 2 weeks ago

@dejan and @jamiecobbett this fix works for us. I'm grateful for your contribution and this gem. We will need to relax the railties restriction as well if we want this to work for rails 8.0.

dejan commented 2 weeks ago

@jamiecobbett thanks for the contribution!

jamiecobbett commented 2 weeks ago

@dejan no problem 👍

I thought I'd have a go at rails 8, and I opened a PR here cc @mahmoudimus