Skip to content

refactor(ui): adopt react-hook-form for remaining auth forms#6606

Open
luannmoreira wants to merge 1 commit into
refactor/adopt-react-hook-formfrom
refactor/rhf-auth-forms
Open

refactor(ui): adopt react-hook-form for remaining auth forms#6606
luannmoreira wants to merge 1 commit into
refactor/adopt-react-hook-formfrom
refactor/rhf-auth-forms

Conversation

@luannmoreira

Copy link
Copy Markdown
Member

What

Migrate the remaining auth and onboarding forms in @shellhub/console onto
the react-hook-form pattern established by the SignUp pilot (#6597): Login,
ForgotPassword, UpdatePassword, Setup, and MfaRecover move to useForm + a
resolver adapter over the existing validators. Stacked on #6597; merge after it.

Why

Advances the Auth & onboarding section of the incremental RHF migration tracked
in shellhub-io/team#152. Each form previously hand-threaded useState +
onChange + touched + memoized validation; this replaces that with the shared
RHF wrappers and per-form resolvers.

Changes

  • resolvers: new loginResolver, forgotPasswordResolver,
    updatePasswordResolver, mfaRecoverResolver (beside their pages) and
    setupResolver (in pages/setup/, alongside validate.ts/signUpResolver.ts).
    Each reuses the existing validators rather than duplicating rules.
  • validateIdentifier: added to utils/validation.ts — login/recover
    identifiers accept a username OR an email, trimmed before validation. The
    login/forgot resolvers return the trimmed value so submit and the 403 redirect
    use it. Login password stays ungated on length so pre-existing credentials
    still authenticate.
  • MfaRecover: binds its bespoke-styled recovery-code <input> via register
    (preserving the yellow focus ring / mono font instead of swapping to
    FormInputField) and reads the value through getValues, so the recovery
    timeout modal's Disable-MFA action still sends the real code. resetField
    replaces the manual state clear on failure.
  • MfaResetRequest: minimal useForm wrapper for a uniform submit path; the
    hook precedes the identifier early-return guard. No field validation (no inputs).
  • Setup: only the step-2 account form moves to RHF; the two-step onboarding
    flow, success redirect, and back button are unchanged.
  • AcceptInvite: intentionally untouched — it is an accept/decline confirmation
    page with no input fields, so RHF has nothing to bind.
  • tests: resolver unit tests + RTL page-behavior tests per form, matching the
    pilot's style.

Testing

vitest, tsc --noEmit, and eslint all run inside the ui container: 198
tests pass across the touched suites. Worth probing: the MfaRecover Disable-MFA
path (must send the actual recovery code, not an empty string) and Login's
username trimming (submit and the 403 confirm-account redirect must use the
trimmed value).

@luannmoreira luannmoreira requested a review from a team as a code owner July 3, 2026 19:32
@luannmoreira luannmoreira self-assigned this Jul 3, 2026
@otavio

otavio commented Jul 3, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 7m 10s —— View job


Code Review Complete

Reviewed 24 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 medium).

The RHF migration is clean and consistent with the SignUp pilot. Hook ordering is correct (useForm precedes every return null guard), formState.isValid is subscribed correctly for the submit-button gating, resolvers return the properly-typed ResolverResult union, and validateIdentifier's username regex matches the server-enforced rule in pkg/validator, so no valid login is newly blocked. Trimming parity is preserved for Login (submit and the 403 confirm-account redirect) and ForgotPassword.

The one finding: in MfaRecover.tsx, handleDisableMfa reads the recovery code via getValues() (untrimmed) while recovery submits the resolver-trimmed value — a whitespace-pasted code can pass recovery but silently fail Disable-MFA. Posted inline with a suggested fix.

Additional notes (non-blocking, not posted inline)

  • Test gapLogin.test.tsx covers username trimming on submit but not on the 403 → /confirm-account redirect, which the PR explicitly calls out as a behavior to protect. Consider adding a 403 case asserting mockNavigate receives the trimmed username.
  • Test assertionMfaRecover.test.tsx's "displays error message on invalid code" manually injects useAuthStore.setState({ error: … }) before asserting, so it passes regardless of whether the failure path actually surfaces the error. (The Disable-MFA "sends the actual recovery code" test, by contrast, is a genuine assertion — good.)
  • Consistency — the four new resolvers live in pages/ root while the pilot placed signUpResolver.ts/setupResolver.ts under pages/setup/. Cosmetic only.

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Comment thread ui/apps/console/src/pages/MfaRecover.tsx Outdated
@luannmoreira

Copy link
Copy Markdown
Member Author

Thanks for the thorough review. Addressed the inline finding plus two of the non-blocking notes in cc82393:

  • 403-redirect trim coverage — added a Login test asserting the 403 → /confirm-account redirect receives the trimmed username.
  • MfaRecover error test — reworked so the mocked recoverWithCode replicates the real store action (sets error and rejects) instead of injecting the error state after submit, so it now genuinely exercises the error-surfacing path.

Leaving the third note (resolver placement under pages/setup/ vs pages/) as-is — cosmetic, and moving them now would add file-move churn to this PR.

luannmoreira added a commit that referenced this pull request Jul 3, 2026
Trim the recovery code in MfaRecover's Disable-MFA call so it matches the
resolver-trimmed value used for recovery; a whitespace-padded code no longer
passes recovery but fails the disable escape hatch.

Strengthen two tests: assert the Login 403 redirect uses the trimmed username,
and drive MfaRecover's error test through the store action's real behavior
instead of injecting the error state after submit.
@otavio

otavio commented Jul 3, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 5m 15s —— View job


Code Review Complete

Reviewed 24 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 low).

The follow-up commit (cc82393) cleanly resolved both previously-raised concerns, verified this round:

  • MfaRecover Disable-MFA trimhandleDisableMfa now sends getValues("recoveryCode").trim(), matching the resolver-trimmed value used for recovery. getValues returns raw field state (resolver transforms aren't written back), so the explicit .trim() is required and correct.
  • MfaRecover error test — the mocked recoverWithCode now replicates the real store action (sets error and rejects), so the assertion genuinely exercises the error-surfacing path rather than injecting state before asserting.

Other areas checked out: hook ordering (useForm precedes every return null guard), resolver ResolverResult return shapes, formState.isValid submit gating, trimming parity for Login (submit and 403 redirect) and ForgotPassword, and validateIdentifier's USERNAME_REGEX (/^[a-z0-9._@-]{3,32}$/) exactly mirroring the backend rule in pkg/validator/validator.go:73 — no valid login is newly blocked. Login password stays ungated on length as intended.

The one finding

validateIdentifier (utils/validation.ts:40) returns the bare fragment "is required" on empty input, whereas every sibling validator returns a full "<Field> is required" sentence. The message renders verbatim, so an empty-and-blurred Login/ForgotPassword identifier field shows just "is required". Posted inline with a suggested fix (and a note that the assertion in validation.test.ts needs the matching update).


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.
refactor/rhf-auth-forms

Comment thread ui/apps/console/src/utils/validation.ts Outdated
@otavio

otavio commented Jul 3, 2026

Copy link
Copy Markdown
Member

@luannmoreira Besides the comments that the code review has raised, I think that you can do the move of the files, as for the git, it's very cheap and are not going to be shown in the DIFF, so it's not going to be hard to review either way.

@luannmoreira

Copy link
Copy Markdown
Member Author

Done in 6805c63 — moved loginResolver, forgotPasswordResolver, updatePasswordResolver, and mfaRecoverResolver (with their tests) under pages/setup/ alongside signUpResolver/setupResolver, and updated the four page imports. As you noted, git tracked them all as pure renames. tsc + eslint clean, 76 affected tests pass.

@otavio

otavio commented Jul 3, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 6m 18s —— View job


Code Review Complete

Reviewed 24 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 low).

The migration is a faithful, security-neutral refactor. I re-verified everything the prior rounds flagged and confirmed the current state is clean:

  • Value-sent-to-backend parity — every form sends the same (or more strictly trimmed) value as pre-migration. Login sends the trimmed username to both login() and the 403 /confirm-account redirect (properly encodeURIComponent'd); ForgotPassword trims account; UpdatePassword/Setup send raw values as before; MfaRecover now trims consistently on both the recover and Disable-MFA calls.
  • Hook orderinguseForm (and all hooks) precede every early return null guard across all six pages. No conditional hooks.
  • Resolvers — all five reuse the existing validators (validate, validatePassword, validateIdentifier) and return the correct ResolverResult union shape matching the SignUp pilot. validateIdentifier's USERNAME_REGEX (/^[a-z0-9._@-]{3,32}$/) is character-for-character identical to the backend rule in pkg/validator/validator.go:73, so no valid login is newly blocked. Submit is gated on formState.isValid.
  • Cross-repocloud/ is Go-only (no TSX); nothing imports these console pages/resolvers, so no enterprise breakage.

The one finding

MfaRecover.test.tsx — the test titled "shows timeout modal after successful recovery" only asserts recoverWithCode was called and never checks that the modal renders, so it stays green even if the modal path breaks. Posted inline with a suggested assertion (the success→modal behavior is separately covered by the disableMfa test, so this one is redundant as well as vacuous).

Non-blocking notes (not posted inline)

  • MfaRecover error UX — unlike its five siblings (which use FormInputField/FormPasswordField and surface per-field errors), MfaRecover binds a raw <input> via register and never reads formState.errors, so the resolver's "Recovery code is required" message is never shown — the user just gets a disabled button. Intentional per the PR (preserves the bespoke input styling), but it's the one spot the migration diverges from its own error-surfacing convention.
  • required attributeLogin dropped the native required prop from its fields while ForgotPassword/UpdatePassword kept it. Harmless (submit is gated on formState.isValid), cosmetic only.

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.
refactor/rhf-auth-forms

Comment thread ui/apps/console/src/pages/__tests__/MfaRecover.test.tsx
@luannmoreira luannmoreira force-pushed the refactor/rhf-auth-forms branch from 67d2ab3 to 5b3c0f4 Compare July 6, 2026 12:36
@otavio

otavio commented Jul 6, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 7m 58s —— View job


Code Review Complete

Reviewed 24 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 low).

This is a faithful, security-neutral RHF migration, and the three items from prior rounds are confirmed fixed in the current source:

  • MfaRecover Disable-MFA trimhandleDisableMfa now sends getValues("recoveryCode").trim(), matching the resolver-trimmed value used for recovery.
  • validateIdentifier empty message — returns the full "Username or email is required" sentence, with the test assertion updated to match.
  • MfaRecover modal test"shows timeout modal after successful recovery" now asserts findByText(/Recovery Window Active/i), no longer vacuous.

Everything else re-verified clean:

  • Backend value parity — every form sends the same (or more strictly trimmed) value as before. Login sends the trimmed username to both login() and the 403 /confirm-account redirect (encodeURIComponent'd); ForgotPassword trims account; UpdatePassword/Setup send raw values as before. Login password stays ungated on length, so pre-existing credentials still authenticate.
  • Validator parityvalidateIdentifier's USERNAME_REGEX (/^[a-z0-9._@-]{3,32}$/) is character-for-character identical to the backend rule in pkg/validator/validator.go:73; no valid login is newly blocked.
  • Hook orderinguseForm (and all hooks) precede every early return null guard across all six pages. No conditional hooks.
  • Resolvers — all five reuse the existing validators and return the correct ResolverResult union shape; submit is gated on formState.isValid. Consistent with the SignUp pilot.
  • Cross-repocloud/ is Go-only; nothing there imports these console pages/resolvers/validation utils, so no enterprise breakage.
  • Tests — no vacuous/tautological assertions found; negative "no error before touch" checks use regexes that genuinely match the rendered messages, and store-error tests exercise the real failure path.

The one finding

Setup.test.tsx reintroduces banner/divider comments (/* --- Mocks --- */, /* === Tests === */, etc.) at four spots, which the project test-style convention says to delete — while this same PR removes the identical banners from Login.test.tsx. Cosmetic, but an internal inconsistency within the diff. Posted inline.


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.
refactor/rhf-auth-forms

Comment thread ui/apps/console/src/pages/__tests__/Setup.test.tsx Outdated
@luannmoreira luannmoreira force-pushed the refactor/rhf-auth-forms branch from 5b3c0f4 to ebed9f6 Compare July 6, 2026 12:57
@otavio

otavio commented Jul 6, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 5m 55s —— View job


Code Review Complete

Reviewed 24 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

All items from the prior four rounds are confirmed fixed in the current source:

  • MfaRecover Disable-MFA trimhandleDisableMfa sends getValues("recoveryCode").trim(), matching the resolver-trimmed value used for recovery.
  • validateIdentifier empty message — returns the full "Username or email is required" sentence, with the test assertion updated to match.
  • MfaRecover modal test"shows timeout modal after successful recovery" asserts findByText(/Recovery Window Active/i); no longer vacuous.
  • Setup.test.tsx banners — all four banner/divider blocks removed, consistent with the cleanup elsewhere in the diff.

Re-verified clean this round:

  • Backend value parity — every form sends the same (or more strictly trimmed) value as before. Login sends the trimmed username to both login() and the encodeURIComponent'd 403 /confirm-account redirect; ForgotPassword trims account; Setup/UpdatePassword send raw values; passwords are never trimmed, so pre-existing credentials still authenticate.
  • Validator parityvalidateIdentifier's USERNAME_REGEX (/^[a-z0-9._@-]{3,32}$/) matches the backend rule in pkg/validator/validator.go, and email login is covered separately, so no valid credential is newly blocked. validateIdentifier is a purely additive export with no pre-existing callers.
  • Hook orderinguseForm and all hooks precede every early return null guard across all six pages. No conditional hooks.
  • Resolvers — all five reuse the existing validators and return the correct ResolverResult union shape, gated on formState.isValid, consistent with the SignUp pilot and now co-located under pages/setup/.
  • Cross-repocloud/ is Go-only; nothing there imports these console pages/resolvers/utils, so no enterprise breakage.
  • Tests — no vacuous/tautological assertions; resolver and page tests assert real error shapes and DOM effects, and store-error tests exercise the real failure path.

If you push additional changes and want a new review, tag @shellhub-io/admin and a team member can trigger it.
refactor/rhf-auth-forms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants