Refactor JS #241

rafaella-martino · 2025-08-15T15:58:44Z

Summary:

Remove credential.js
~~Remove messenger.js~~
Centralize JS logic in one controller
Handle form responses in Rails controllers (when possible)
Resolve deprecation warning
Display error messages using alerts

…ay errors

… public_key_credential

app/views/home/index.html.erb

app/views/registrations/new.html.erb

app/controllers/registrations_controller.rb

nicolastemciuc · 2025-08-15T17:06:46Z

app/views/layouts/application.html.erb

-
-    <div class="mdc-snackbar js-messenger">
-      <div class="mdc-snackbar__surface">
-        <div class="mdc-snackbar__label" role="status" aria-live="polite">
-        </div>
-
-        <div class="mdc-snackbar__actions">
-          <button type="button" title="dismiss" class="mdc-icon-button material-icons mdc-snackbar__dismiss">close</button>
-        </div>
-      </div>
-    </div>


Why we stop using the snackbar to show the messages?

Because we had to keep messenger.js, and all its logic, that don't seem very useful...
Using alerts is less code and the user experience is very similar 🙂.
Do you think we should keep the snackbar though?

I’m not convinced they’re all that similar

The snackbar feels consistent with the other demo UI components, and since it has a timeout, it doesn’t require the user to manually dismiss it

I would try to keep it if we can – removing it seems out of the scope of the PR as I think the idea was to refactor the way the browser and our application interact with each other when performing a webauthn cermony

.rubocop.yml

nicolastemciuc · 2025-08-15T17:09:56Z

app/javascript/controllers/add_credential_controller.js

      }
-    });
+    } catch (error) {
+      alert(error.message || error);


is it okay to show the error if there's no message?

Maybe we could add a generic error message? Like the one we set before:

"Sorry, something wrong happened."

Sounds good!

santiagorodriguez96

Looking great! Thank you ❤️

app/controllers/credentials_controller.rb

app/controllers/sessions_controller.rb

app/javascript/controllers/webauthn_credential_controller.js

santiagorodriguez96 · 2025-08-15T21:06:29Z

app/views/layouts/application.html.erb

-
-    <div class="mdc-snackbar js-messenger">
-      <div class="mdc-snackbar__surface">
-        <div class="mdc-snackbar__label" role="status" aria-live="polite">
-        </div>
-
-        <div class="mdc-snackbar__actions">
-          <button type="button" title="dismiss" class="mdc-icon-button material-icons mdc-snackbar__dismiss">close</button>
-        </div>
-      </div>
-    </div>


I would try to keep it if we can – removing it seems out of the scope of the PR as I think the idea was to refactor the way the browser and our application interact with each other when performing a webauthn cermony

santiagorodriguez96 · 2025-08-15T21:07:05Z

app/views/registrations/new.html.erb

-              <%= form.submit "Register using WebAuthn", class: "mdc-button mdc-button--raised" %>
+              <%= button_tag "Register using WebAuthn", type: "submit", class: "mdc-button mdc-button--raised" %>


Why not using form.submit?

When using form.submit, if we clicked Cancel when adding a new Credential the button was disabled.
This is because when using rails helper form.submit, the button stays disabled until the submit flow is over.
So, as we use prevent to intercept the controllers action, we should not use this helper 🙂 .

Another option could be to re-enable the button from the javascript when it fails 🤔

Another option could be to re-enable the button from the javascript when it fails 🤔

I like this!

app/assets/stylesheets/application.css

`options`

nicolastemciuc · 2025-08-18T18:18:21Z

app/javascript/controllers/webauthn_credential_controller.js

+
+  async get() {
+    try {
+      const optionsResponse = await fetch(this.optionsUrlValue, {


what do you think about naming it response?

1c92613
Like it, I also changed optionsJson to credentialOptionsJson.
Are you okay with this change or is it too long?
As this is a Demo I believe variables should be understandable 🙂

nicolastemciuc · 2025-08-18T18:40:39Z

app/controllers/registrations_controller.rb

+        redirect_to root_path, notice: "Security Key registered successfully"
      else
-        render json: "Couldn't register your Security Key", status: :unprocessable_content
+        redirect_to new_registration_path, alert: "Couldn't register your Security Key"


should we use render here? I think that when we use redirect_to it responds with a 302 status code

See:

https://www.mintbit.com/blog/difference-between-render-and-redirect-to-in-rails-controllers/

https://api.rubyonrails.org/classes/ActionController/Redirecting.html#method-i-redirect_to:~:text=The%20status%20code%20can%20either%20be%20a%20standard%20HTTP%20Status%20code%20as%20an%20integer%2C%20or%20a%20symbol%20representing%20the%20downcased%2C%20underscored%20and%20symbolized%20description.%20Note%20that%20the%20status%20code%20must%20be%20a%203xx%20HTTP%20code%2C%20or%20redirection%20will%20not%20occur.

nicolastemciuc · 2025-08-18T18:40:53Z

app/controllers/credentials_controller.rb

+        redirect_to root_path, notice: "Security Key registered successfully"
      else
-        render json: "Couldn't add your Security Key", status: :unprocessable_content
+        redirect_to root_path, alert: "Couldn't register your Security Key"


nicolastemciuc · 2025-08-18T18:42:12Z

app/controllers/sessions_controller.rb

when the sign in fails it renders:

takes place

alerts and notices

santiagorodriguez96 · 2025-09-01T20:18:55Z

app/javascript/application.js

 import Rails from "@rails/ujs";

 Rails.start();
+import "@rails/request.js"


nit: should we move this up along with the rest of the imports?

santiagorodriguez96 · 2025-09-01T20:33:16Z

app/controllers/credentials_controller.rb

    rescue WebAuthn::Error => e
-      render json: "Verification failed: #{e.message}", status: :unprocessable_content
+      flash[:alert] = "Verification failed: #{e.message}"
+      render "home/index", status: :unprocessable_content


I don't really like this pattern to be honest... 🤔

If something goes wrong when adding a passkey, the index page will be rendered but the url is /credentials so reloading the page will show the typical Confirm form resubmission alert

Not the end of the world but I wonder if there are other options here

Yes, we can find another way!

First, we used a redirect to handle this error flow. However, we changed it because the resulting status was 301, which didn’t reflect the unprocessable_content status we wanted in case of an exception.

But I believe the possibility of the user seeing the “Confirm form resubmission” prompt should be avoided. So maybe we should use the redirect with an alert again.

What do you think? 🙂

(pd: the alerts are not yet added to the index page, it is a will do)

However, by doing this we won't be having some log in the server informing this error...

449c9cb

I temporarily used redirect to handle this case, avoiding the alert when the user reloads the page.
Maybe this can be adjusted in another PR so it does not block this one?

What do you think about keeping the behavior of the controllers as it was before and revisit the approach as part of a separate PR?

I was thinking about doing it for all the responses of all the controllers. What do you think?

That'd make it so that all the controllers will still respond using json for now.

santiagorodriguez96 · 2025-09-01T20:34:26Z

app/controllers/credentials_controller.rb

      if credential.update(
-        nickname: params[:credential_nickname],
+        nickname: credential_params[:nickname],
        public_key: webauthn_credential.public_key,
        sign_count: webauthn_credential.sign_count
      )
-        render json: { status: "ok" }, status: :ok
+        flash[:notice] = "Security Key registered successfully"
+        redirect_to root_path
      else
-        render json: "Couldn't add your Security Key", status: :unprocessable_content
+        flash[:alert] = "Couldn't register your Security Key"
+        render "home/index", status: :unprocessable_content


If something fails here, the index page will be rendered with a phantom credential (as we are initializing a credential for the user) that will go away when the page is refreshed

You're right!
We could destroy that instance in case the update action fails. I believe another solution would implicate more lines of code. What do you think? 🙂

commit

I think the problem is when the credential is not persisted (because it was just initialized) so I don't think that fixes the issue 😕

You're right, the credential is a new record. What do you think about using this method?
9e58703

Now that we added back the redirect I don't think this is a problem anymore 🙂

Indeed!
23a316e

destroy initialized credential in case `create` credential action fails

targets

with json

santiagorodriguez96 · 2025-10-13T21:14:08Z

app/controllers/registrations_controller.rb

      end
    rescue WebAuthn::Error => e
-      render json: "Verification failed: #{e.message}", status: :unprocessable_content
+      render json: { message: "Verification failed: #{e.message}", redirect_to: registration_path },


I'm getting an error here – we should be using new_registration_path instead

santiagorodriguez96 · 2025-10-13T21:14:38Z

app/controllers/sessions_controller.rb

+      render json: { message: "Security Key authenticated successfully", redirect_to: root_path }, status: :ok
    rescue WebAuthn::Error => e
-      render json: "Verification failed: #{e.message}", status: :unprocessable_content
+      render json: { message: "Verification failed: #{e.message}", redirect_to: session_path },


Same here! We should use new_session_path here instead

santiagorodriguez96 · 2025-10-13T21:15:08Z

app/controllers/sessions_controller.rb

+      render json: { message: "Security Key authenticated successfully", redirect_to: root_path }, status: :ok
    rescue WebAuthn::Error => e
-      render json: "Verification failed: #{e.message}", status: :unprocessable_content
+      render json: { message: "Verification failed: #{e.message}", redirect_to: session_path },


I'm not getting any of the messages rendered in the page 😕

rafaella-martino added 12 commits August 13, 2025 14:39

feat: remove credential.js

ac998c2

feat: remove ajax, rename controllers actions, use js alerts to displ…

1532925

…ay errors

feat: remove message.js as it is no longer used

cb11999

feat: remove unused js targets

521b0c2

feat: replace fetch for submit, change credential param name to…

7bddc67

… public_key_credential

test: update test with renamed params

871ab25

feat: change .then for await for readability

cbf503e

feat: add permitted params in controllers

a990e01

feat: pass url as values, add token to session js controller

bd72e31

feat: merge js controllers into one

6115812

feat: change submit buttons, rename js controller variables

e19ef24

fix: attend deprecation warning

9b46e85

rafaella-martino requested a review from nicolastemciuc August 15, 2025 15:58

Merge branch 'master' into rm--refactor_js

6fe61ca

nicolastemciuc reviewed Aug 15, 2025

View reviewed changes

rafaella-martino added 2 commits August 15, 2025 14:41

style: use rails button helpers

6002921

feat: use expect to require params

e93e3e8

rafaella-martino requested a review from nicolastemciuc August 15, 2025 18:28

santiagorodriguez96 reviewed Aug 15, 2025

View reviewed changes

rafaella-martino added 4 commits August 18, 2025 13:27

feat: change create_options and get_options controller actions for

95b5664

`options`

fix: add space in css

331859e

rollback: use messenger.js to display error messages

c42c9fc

feat: use requestjs gem

7a35c2c

rafaella-martino requested a review from santiagorodriguez96 August 18, 2025 17:44

nicolastemciuc reviewed Aug 18, 2025

View reviewed changes

rafaella-martino added 3 commits August 18, 2025 16:04

fix: Display layout instead of json when Webauthn Verification error

81ad153

takes place

feat: use render instead of redirect when request fails, use flash for

c2753ed

alerts and notices

feat: Change variable names in JS controller

1c92613

rafaella-martino requested a review from nicolastemciuc August 19, 2025 14:32

santiagorodriguez96 reviewed Sep 1, 2025

View reviewed changes

rafaella-martino added 13 commits September 3, 2025 11:53

refactor: move request.js import with the rest of the imports

5307cb1

refactor: temporarily use redirect when create credential fails,

449c9cb

destroy initialized credential in case `create` credential action fails

refactor: enabling submit button when clicking Cancel with js

e8b7ea3

targets

feat: clear not saved credential if there is an error

9e58703

Merge branch 'master' into rm--refactor_js

672bcb1

make rubocop happy

b9edbe2

refactor: remove unnecessary cleanup when error updating credential

23a316e

rollback handling Webauthn Error exception

55e10a6

refactor: rollback to use json responses in case of errors

e35488d

refactor: leave controller response for next PR, rollback to respond

3accfb1

with json

change response message if registration success

18b76a5

test: add new controller changes

72e5f40

stop displaying message

82a766f

rafaella-martino requested a review from santiagorodriguez96 September 12, 2025 19:26

santiagorodriguez96 reviewed Oct 13, 2025

View reviewed changes

		<%= form.submit "Register using WebAuthn", class: "mdc-button mdc-button--raised" %>
		<%= button_tag "Register using WebAuthn", type: "submit", class: "mdc-button mdc-button--raised" %>

Refactor JS #241

Are you sure you want to change the base?

Refactor JS #241

Uh oh!

Conversation

rafaella-martino commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaella-martino Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

santiagorodriguez96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaella-martino Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

rafaella-martino commented Aug 15, 2025 •

edited

Loading

rafaella-martino Aug 15, 2025 •

edited

Loading

rafaella-martino Aug 18, 2025 •

edited

Loading

santiagorodriguez96 Sep 3, 2025 •

edited

Loading

santiagorodriguez96 Sep 11, 2025 •

edited

Loading

santiagorodriguez96 Oct 13, 2025 •

edited

Loading