Fix flaky reset password test with custom Turbo Stream visit action

[maebeale]

Closes #1473

What is the goal of this PR and why is this important?

• Fix intermittent CI failure in reset_password_person_spec.rb where the user ends up at /users/sign_in instead of / after changing their password
• Fix confirm dialog failures in both reset_password_person_spec.rb and change_password_flow_spec.rb

How did you approach the change?

Password change form (flaky redirect to sign_in):

• Turbo 8's fetch()-based form submission with redirect: "follow" has a race condition where the browser may not process the Set-Cookie header from a 303 redirect before making the follow-up GET request — causing Devise to invalidate the session due to a stale authenticatable_salt (which changes when the password changes via update_with_password)
• Added a custom Turbo Stream visit action that separates session update from navigation: the server responds with a 200 (not a redirect) containing a <turbo-stream action="visit"> tag, the browser processes the Set-Cookie header, then the custom action triggers Turbo.visit() client-side
• turbo_stream_visit(url) helper in ApplicationController generates the stream tag; Turbo.StreamActions.visit in turbo-events.js handles it client-side
• This pattern can be reused anywhere a form submission modifies the session and needs to redirect

"Log out and reset it." link (confirm dialog test failures):

• Fixed change_password_flow_spec.rb to use accept_confirm (wrapping the click) instead of accept_alert (after the click)
• Added JS-ready waits (have_css("[data-controller='password-toggle']")) to both specs to ensure Turbo is initialized before clicking the confirm link

UI Testing Checklist

• Change password with correct current password → redirects to home, stays logged in
• Change password with wrong current password → re-renders form with errors
• "Log out and reset it." link shows confirm dialog and redirects to password reset page

Anything else to add?

• Reordered test assertions to check flash content before path — flash presence is a stronger synchronization point than URL check
• The custom Turbo Stream visit action is a general solution for the Turbo/session race condition — prefer it over data-turbo="false" which disables Turbo for all descendant elements

🤖 Generated with Claude Code

[maebeale]

This custom Turbo Stream action is the client-side counterpart to turbo_stream_visit in ApplicationController. When the server responds with <turbo-stream action="visit" url="/">, this handler triggers Turbo.visit() — ensuring the browser has already processed the Set-Cookie header from the 200 response before navigation begins.

[maebeale]

This helper generates a raw Turbo Stream tag rather than using the turbo_stream builder because we need a custom action (visit) not in the standard set. The ERB::Util.html_escape prevents XSS if the URL somehow contains user input.

[maebeale]

The key change: instead of redirect_to (which Turbo follows via fetch, racing against Set-Cookie processing), we respond with a 200 containing a Turbo Stream that triggers client-side navigation. The browser reliably processes Set-Cookie on the 200 response before the stream action fires.

[jmilljr24]

I need to look into it more but these changes make me pause for a second.

Do we actually have an issue with updating passwords or it just the test being flaky? I don't love the idea of adding special code just to make a test past when the application works.

I can see how this change is intended to work but it seems like we are reinventing the wheel. My gut says that a devise password update with turbo and keeping the user logged in is a pretty normal flow.

Here is the default update action from the devise controller. We of course have our own action with some custom stuff like ahoy but for the most part there is nothing special in devise.

  def update
    self.resource = resource_class.to_adapter.get!(send(:"current_#{resource_name}").to_key)
    prev_unconfirmed_email = resource.unconfirmed_email if resource.respond_to?(:unconfirmed_email)

    resource_updated = update_resource(resource, account_update_params)
    yield resource if block_given?
    if resource_updated
      set_flash_message_for_update(resource, prev_unconfirmed_email)
      bypass_sign_in resource, scope: resource_name if sign_in_after_change_password?

      respond_with resource, location: after_update_path_for(resource)
    else
      clean_up_passwords resource
      set_minimum_password_length
      respond_with resource
    end
  end

Maybe respond_with from the responders gem. But again, I'm wonder if this is just a test issue not an app issue.

[maebeale]

I need to look into it more but these changes make me pause for a second.

Do we actually have an issue with updating passwords or it just the test being flaky? I don't love the idea of adding special code just to make a test past when the application works.

I can see how this change is intended to work but it seems like we are reinventing the wheel. My gut says that a devise password update with turbo and keeping the user logged in is a pretty normal flow.

Here is the default update action from the devise controller. We of course have our own action with some custom stuff like ahoy but for the most part there is nothing special in devise.

  def update
    self.resource = resource_class.to_adapter.get!(send(:"current_#{resource_name}").to_key)
    prev_unconfirmed_email = resource.unconfirmed_email if resource.respond_to?(:unconfirmed_email)

    resource_updated = update_resource(resource, account_update_params)
    yield resource if block_given?
    if resource_updated
      set_flash_message_for_update(resource, prev_unconfirmed_email)
      bypass_sign_in resource, scope: resource_name if sign_in_after_change_password?

      respond_with resource, location: after_update_path_for(resource)
    else
      clean_up_passwords resource
      set_minimum_password_length
      respond_with resource
    end
  end

Maybe respond_with from the responders gem. But again, I'm wonder if this is just a test issue not an app issue.

i tagged you in to get your thoughts bc i wasn't sold either. yeah, the test is often flaky and so just trying to get us with a more reliable suite.

[jmilljr24]

I need to look into it more but these changes make me pause for a second.
Do we actually have an issue with updating passwords or it just the test being flaky? I don't love the idea of adding special code just to make a test past when the application works.
I can see how this change is intended to work but it seems like we are reinventing the wheel. My gut says that a devise password update with turbo and keeping the user logged in is a pretty normal flow.
Here is the default update action from the devise controller. We of course have our own action with some custom stuff like ahoy but for the most part there is nothing special in devise.

  def update
    self.resource = resource_class.to_adapter.get!(send(:"current_#{resource_name}").to_key)
    prev_unconfirmed_email = resource.unconfirmed_email if resource.respond_to?(:unconfirmed_email)

    resource_updated = update_resource(resource, account_update_params)
    yield resource if block_given?
    if resource_updated
      set_flash_message_for_update(resource, prev_unconfirmed_email)
      bypass_sign_in resource, scope: resource_name if sign_in_after_change_password?

      respond_with resource, location: after_update_path_for(resource)
    else
      clean_up_passwords resource
      set_minimum_password_length
      respond_with resource
    end
  end

Maybe respond_with from the responders gem. But again, I'm wonder if this is just a test issue not an app issue.

i tagged you in to get your thoughts bc i wasn't sold either. yeah, the test is often flaky and so just trying to get us with a more reliable suite.

Thanks for diving into it!

My vote would be disable this specific test for now and open an issue for it. Maybe someone will pick it up. We know the password change works and would surface pretty quickly from user feedback if it wasn't. And it seems like the only thing in question regarding the test is the redirect after updating, not the update itself.

[maebeale]

ok, @jmilljr24 i'm going to close this pr but leave the test in and see how flaky it actually is.