Port login page to preact #9589

mtomilov · 2025-05-26T17:42:37Z

Here we port login page to react in a hybrid way.
React app renders the form, but we actually submit it. And POST request is handled by the backend.
Login form controller is mostly based on group form controller.
For now it only affects regular login and open authorization popup login still uses Jinja2 for easier porting, which we'll address next.
Reusable components like text field will need to be lifted up from group form app.

Testing

Go to /login page
Try logging with valid and invalid credentials
Go to /login?for_oauth=true page
Try logging with valid and invalid credentials to make sure it continues to work as before

h/static/scripts/login-forms/components/AppRoot.tsx

robertknight · 2025-05-27T07:53:04Z

For the files which are re-used unchanged from the group forms app, I would suggest to simply re-import them from there for the moment. One of the frontend devs can then look at moving them to a shared directory and adjusting the tests etc.

mtomilov · 2025-05-27T08:09:48Z

For the files which are re-used unchanged from the group forms app, I would suggest to simply re-import them from there for the moment. One of the frontend devs can then look at moving them to a shared directory and adjusting the tests etc.

Thank you. I was going to lift them up to /scripts, but sure not problem.
Duplicated them for draft because I wanted to make sure that I can reuse them as is.

mtomilov · 2025-05-28T13:16:05Z

h/static/scripts/group-forms/components/forms/TextField.tsx

+  inputType?: 'text' | 'password';
+
+  /** Name of the input field. */
+  name?: string;


We need to make some changes for text properties because we actually submit this React form, not make API calls from here.

mtomilov · 2025-05-28T13:16:43Z

h/static/scripts/group-forms/components/forms/TextField.tsx

@@ -60,6 +74,9 @@ export type TextFieldProps = {

  /** Additional classes to apply to the input element. */
  classes?: string;
+
+  /** Optional error message for the field. */
+  fieldError?: string;


And this is field validation error we get from backend login schema.
I've tried to make regular input error work but unfortunately it seems to be designed for in-place submit validation, not when the error is coming from the outside.

mtomilov · 2025-05-28T13:17:15Z

h/static/scripts/group-forms/components/forms/TextField.tsx

+        {fieldError && (
+          <>
+            <div className="grow" />
+            <ErrorNotice message={fieldError} />


Conditionally rendering the field error

mtomilov · 2025-05-28T13:17:53Z

h/static/scripts/group-forms/components/forms/TextField.tsx

        classes={classes}
        autofocus={autofocus}
        autocomplete="off"
        required={required}
+        {...(name ? { name: name } : {})}
+        {...(inputType === 'password' ? { type: 'password' } : {})}


This works, but I'm not a fan. Maybe we actually need a separate text field for login/signup form, but the changes seem to be minor, so I'm not sure.

You can set these fields but use the value undefined and they won't appear in the HTML:

diff --git a/h/static/scripts/group-forms/components/forms/TextField.tsx b/h/static/scripts/group-forms/components/forms/TextField.tsx index 0925b7177..bea45fcfe 100644 --- a/h/static/scripts/group-forms/components/forms/TextField.tsx +++ b/h/static/scripts/group-forms/components/forms/TextField.tsx @@ -78,7 +78,7 @@ export type TextFieldProps = { */ export default function TextField({ type = 'input', - inputType = 'text', + inputType, value, onChangeValue, minLength = 0, @@ -87,7 +87,7 @@ export default function TextField({ required = false, autofocus = false, classes = '', - name = '', + name, fieldError = '', }: TextFieldProps) { const id = useId(); @@ -131,8 +131,8 @@ export default function TextField({ autofocus={autofocus} autocomplete="off" required={required} - {...(name ? { name: name } : {})} - {...(inputType === 'password' ? { type: 'password' } : {})} + name={name} + type={inputType} /> {typeof maxLength === 'number' && ( <CharacterCounter

Perfect, thank you!

mtomilov · 2025-05-28T13:19:00Z

h/static/scripts/login-forms/components/AppRoot.tsx

+      <Config.Provider value={config}>
+        <Router>
+          <Switch>
+            <Route path={routes.login}>


I'm thinking to extend this with sign up route in the switch when we get to that rather than creating a new sign up react app.

Yes, I think that will make sense.

mtomilov · 2025-05-28T13:20:12Z

h/static/scripts/login-forms/components/LoginForm.tsx

+        data-testid="form"
+        className="max-w-[530px] mx-auto flex flex-col gap-y-4"
+      >
+        <input type="hidden" name="csrf_token" value={config.csrfToken} />


Setting csrf token from js config on the form for pyramid.

mtomilov · 2025-05-28T13:23:09Z

h/templates/accounts/login.html.jinja2

+    </footer>
+  </div>
+  </div>
+{% endblock %}


This mostly follows what we have in group jinja2 html template.

mtomilov · 2025-05-28T13:24:40Z

h/views/accounts.py

    @view_config(
        request_method="GET",
        request_param="for_oauth",
        renderer="h:templates/accounts/login_oauth.html.jinja2",
    )
-    def get(self):
-        """Render the login page, including the login form."""
+    def get_oauth(self):


Splitting these two routes for now to limit the scope of this PR.
So, regular login is covered by React app and open authorization login is covered by Jinja2 at this point.

mtomilov · 2025-05-28T13:25:46Z

h/views/accounts.py

+        csrf_token = get_csrf_token(self.request)
+
+        return {
+            "styles": self.request.registry["assets_env"].urls("group_forms_css"),


Reusing group form styles because basically these are just tailwind styles, which is exactly the same we need for login form.

Something to know about how Tailwind works is that it scans a specified set of files looking for Tailwind classes that are used and only generates those styles in the CSS. The set of files that are scanned includes all of our TSX files under h/static/scripts, so re-using the CSS here will work. It will probably make sense to rename this bundle to something more general.

Sure, renamed to forms.css

mtomilov · 2025-05-28T13:26:35Z

h/views/accounts.py

+        except deform.ValidationFailure as e:
+            js_config = self._js_config()
+            js_config["formErrors"] = e.error.asdict()
+            js_config["formData"] = {


We need to pass back both form errors and form data for pre-fill if backend POST validation fails.
Again, this is a separate route to take care of regular POST login for now.
I'm using camel case here because this data is prepared by backend but used by frontend.

mtomilov · 2025-05-28T13:28:56Z

tests/functional/accounts_test.py

-        res.form["username"] = user.username
-        res.form["password"] = "pass"  # noqa: S105
-        res.form.submit()
+        js_config = json.loads(res.html.find("script", class_="js-config").text)


Since login form is now rendered by React, we need to make POST request here for login, taking csrf token from HTML page body.

robertknight · 2025-05-28T14:45:25Z

For the benefit of non-developers, the old login screen:

New login screen:

robertknight

This is looking good overall from an initial glance. I think we should remove the visible "required" markers from the fields in this form, though we will need to keep the required attribute on the form field. This will require a small change to the TextField API.

robertknight · 2025-05-28T14:52:15Z

h/static/scripts/group-forms/components/forms/TextField.tsx

        classes={classes}
        autofocus={autofocus}
        autocomplete="off"
        required={required}
+        {...(name ? { name: name } : {})}
+        {...(inputType === 'password' ? { type: 'password' } : {})}


You can set these fields but use the value undefined and they won't appear in the HTML:

diff --git a/h/static/scripts/group-forms/components/forms/TextField.tsx b/h/static/scripts/group-forms/components/forms/TextField.tsx index 0925b7177..bea45fcfe 100644 --- a/h/static/scripts/group-forms/components/forms/TextField.tsx +++ b/h/static/scripts/group-forms/components/forms/TextField.tsx @@ -78,7 +78,7 @@ export type TextFieldProps = { */ export default function TextField({ type = 'input', - inputType = 'text', + inputType, value, onChangeValue, minLength = 0, @@ -87,7 +87,7 @@ export default function TextField({ required = false, autofocus = false, classes = '', - name = '', + name, fieldError = '', }: TextFieldProps) { const id = useId(); @@ -131,8 +131,8 @@ export default function TextField({ autofocus={autofocus} autocomplete="off" required={required} - {...(name ? { name: name } : {})} - {...(inputType === 'password' ? { type: 'password' } : {})} + name={name} + type={inputType} /> {typeof maxLength === 'number' && ( <CharacterCounter

robertknight · 2025-05-28T14:52:57Z

h/static/scripts/login-forms/components/AppRoot.tsx

+      <Config.Provider value={config}>
+        <Router>
+          <Switch>
+            <Route path={routes.login}>


Yes, I think that will make sense.

robertknight · 2025-05-28T14:54:17Z

h/static/scripts/login-forms/components/LoginForm.tsx

+          <span>
+            {/* These are in a child span to avoid a gap between them. */}
+            <Star />
+            &nbsp;Required


For a login form I think it is sufficiently obvious that the username and password are required that we can omit this visual indicator. We do need to make sure that the required field is still set on the <input> though.

Yep, should've made it the same as the original form rather than taking that from group form. Thanks.

robertknight · 2025-05-28T14:59:43Z

h/static/scripts/login-forms/config.ts

+};
+
+/** Return the frontend config from the page's <script class="js-config">. */
+export function readConfig(): ConfigObject {


I think it will make sense to re-use the readConfig function from the group forms module, and make that generic, but this can be done as part of some refactoring after this lands.

robertknight · 2025-05-28T15:00:24Z

h/static/scripts/login-forms/index.tsx

+
+function init() {
+  const shadowHost = document.querySelector('#login-form')!;
+  const shadowRoot = shadowHost.attachShadow({ mode: 'open' });


I suspect we can simplify things for ourselves by removing the use of shadow DOM in this form, but that's out of scope for this PR.

robertknight · 2025-05-28T15:02:58Z

h/static/styles/login-forms.css

@@ -0,0 +1,6 @@
+/* CSS entry point for the login app. */


This file appears to be unused.

Yep, removed

robertknight · 2025-05-28T15:06:16Z

h/views/accounts.py

+        csrf_token = get_csrf_token(self.request)
+
+        return {
+            "styles": self.request.registry["assets_env"].urls("group_forms_css"),


Something to know about how Tailwind works is that it scans a specified set of files looking for Tailwind classes that are used and only generates those styles in the CSS. The set of files that are scanned includes all of our TSX files under h/static/scripts, so re-using the CSS here will work. It will probably make sense to rename this bundle to something more general.

mtomilov · 2025-05-28T17:43:35Z

This is looking good overall from an initial glance. I think we should remove the visible "required" markers from the fields in this form, though we will need to keep the required attribute on the form field. This will require a small change to the TextField API.

Added separate showRequired argument, defaulting to required which we set in LoginForm.
Probably we want opt-out behavior for the indication here.

github-advanced-security bot found potential problems May 26, 2025

View reviewed changes

h/static/scripts/login-forms/components/AppRoot.tsx Dismissed Show dismissed Hide dismissed

mtomilov force-pushed the port-login-page-to-preact branch 7 times, most recently from 10962d0 to da90554 Compare May 28, 2025 13:11

mtomilov commented May 28, 2025

View reviewed changes

mtomilov force-pushed the port-login-page-to-preact branch from da90554 to 64122dc Compare May 28, 2025 13:21

mtomilov commented May 28, 2025

View reviewed changes

mtomilov force-pushed the port-login-page-to-preact branch 4 times, most recently from 6d33dbe to fe871c4 Compare May 28, 2025 14:07

mtomilov marked this pull request as ready for review May 28, 2025 14:13

mtomilov requested a review from robertknight May 28, 2025 14:13

mtomilov force-pushed the port-login-page-to-preact branch from fe871c4 to cac877b Compare May 28, 2025 14:19

robertknight reviewed May 28, 2025

View reviewed changes

mtomilov force-pushed the port-login-page-to-preact branch 3 times, most recently from 1544491 to 68ea2f0 Compare May 28, 2025 17:20

Port login page to preact

671f84b

mtomilov force-pushed the port-login-page-to-preact branch from 68ea2f0 to 671f84b Compare May 28, 2025 17:28

mtomilov requested a review from robertknight May 28, 2025 17:45

Port login page to preact #9589

Are you sure you want to change the base?

Port login page to preact #9589

Uh oh!

Conversation

mtomilov commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

Uh oh!

robertknight commented May 27, 2025

Uh oh!

mtomilov commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtomilov May 28, 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!

mtomilov May 28, 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!

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!

mtomilov May 28, 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!

mtomilov May 28, 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!

mtomilov May 28, 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!

robertknight commented May 28, 2025

Uh oh!

robertknight left a comment

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!

mtomilov commented May 26, 2025 •

edited

Loading

mtomilov commented May 27, 2025 •

edited

Loading

mtomilov May 28, 2025 •

edited

Loading

mtomilov May 28, 2025 •

edited

Loading

mtomilov May 28, 2025 •

edited

Loading

mtomilov May 28, 2025 •

edited

Loading

mtomilov May 28, 2025 •

edited

Loading

mtomilov commented May 28, 2025 •

edited

Loading