refactor(registration): reduce RegisterForm complexity by extracting hooks and subcomponents #38365
Conversation
…subcomponents - Extract form validation logic into useRegisterFormValidation hook - Extract error handling logic into useRegisterErrorHandler hook - Create FormFieldInput component for reusable form fields - Create PasswordFieldWithVerifier component for password fields - Refactor RegisterForm to use new hooks and components - Remove eslint-disable complexity comment This refactoring reduces RegisterForm.tsx from 316 to 171 lines (-145 lines) and improves maintainability by separating concerns.
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughRefactors RegisterForm by extracting validation into Changes
Sequence Diagram(s)mermaid User->>RegisterForm: fill fields & submit Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
MartinSchoeler
left a comment
MartinSchoeler
left a comment
There was a problem hiding this comment.
Can you add some unit tests to the component to make sure the behavior and visual is still the same?
|
Yes, that makes sense, |
…refactoring - Add RegisterForm.spec.tsx with 26+ tests covering: * Rendering all required and conditional fields * Validation for all form fields * Password confirmation matching * Form submission behavior * Accessibility attributes - Add useRegisterFormValidation.spec.tsx with 7 tests covering: * Validation rules generation * Settings-based conditional requirements * Password matching logic - Add useRegisterErrorHandler.spec.tsx with 9 tests covering: * All error scenarios (invalid email, duplicate username, etc.) * Toast message dispatching * Login route redirects - Add FormFieldInput.spec.tsx with 10 tests covering: * Text and textarea rendering * Required field handling * Error display and accessibility - Add PasswordFieldWithVerifier.spec.tsx with 10 tests covering: * Password and confirmation field rendering * Conditional confirmation field * Password validation state * Error handling All tests use @testing-library/react and mockAppRoot for proper isolation and follow existing project testing patterns.
|
Update on tests: I added comprehensive unit tests (56 tests across RegisterForm, related hooks, and components) to validate that the refactor didn’t introduce any behavioral or rendering regressions. While working on this, I found that web-ui-registration doesn’t currently have Jest infrastructure (no test script or preset), and attempts to wire it up using @rocket.chat/jest-presets run into module resolution issues. This appears to be existing technical debt in this package rather than an issue with the tests themselves. To still validate the refactor, I ran the tests locally with a temporary Jest setup, and all tests passed. I’m happy to: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/web-ui-registration/src/RegisterForm.spec.tsx`:
- Around line 356-366: The test currently only checks for a generic form and can
pass if CustomFieldsForm is removed; update the RegisterForm.spec.tsx to
explicitly assert that CustomFieldsForm is rendered by mocking either the
useAccountsCustomFields hook or the CustomFieldsForm component: replace or mock
CustomFieldsForm (or stub the useAccountsCustomFields hook) to render a unique
sentinel element (e.g., data-testid or a distinctive text) and then render
RegisterForm and assert screen.getByTestId or getByText for that sentinel;
reference the RegisterForm component and the
CustomFieldsForm/useAccountsCustomFields symbols when adding the mock and the
assertion so the test fails if the integration is removed.
- Around line 7-9: The tests currently use the global userEvent instance; change
to creating a per-test instance by calling userEvent.setup() in a beforeEach and
storing the returned user (e.g., const user = userEvent.setup()) for all
interactions in that test; update usages in RegisterForm.spec.tsx to call
methods on that local user instead of the global userEvent, and keep
mockSetLoginRoute and mockRegisterMethod as-is to avoid shared interaction state
across tests.
🧹 Nitpick comments (6)
packages/web-ui-registration/src/hooks/useRegisterErrorHandler.spec.tsx (1)
34-46: Consider adding a test forerror-invalid-emailviaerrorType.The hook implementation checks both
error.erroranderror.errorTypefor the invalid email case (line 23 in the hook:[error.error, error.errorType].includes('error-invalid-email')), but only theerror.errorpath is tested here. Adding a test for{ errorType: 'error-invalid-email' }would provide complete coverage of this branch.💡 Suggested additional test
+ it('should handle error-invalid-email error via errorType', () => { + const { result } = renderHook(() => useRegisterErrorHandler(mockSetError, mockSetServerError, mockSetLoginRoute), { + wrapper: defaultAppRoot, + }); + + const error = { errorType: 'error-invalid-email' }; + result.current.handleRegisterError(error); + + expect(mockSetError).toHaveBeenCalledWith('email', { + type: 'invalid-email', + message: 'Invalid email format', + }); + }); + it('should handle error-user-already-exists error', () => {packages/web-ui-registration/src/components/FormFieldInput.spec.tsx (2)
56-66: Consider using a properly typed error mock to avoidas any.The
as anytype assertion bypasses type checking. Consider creating a typed helper or importing theFieldErrortype fromreact-hook-form.♻️ Suggested improvement
+import type { FieldError } from 'react-hook-form'; + +const createFieldError = (type: string, message: string): FieldError => ({ type: type as any, message }); + // Then in tests: -const error = { type: 'required', message: 'This field is required' }; -<FormFieldInput label='Test Field' fieldId='test-id' error={error as any} register={mockRegister} /> +const error = createFieldError('required', 'This field is required'); +<FormFieldInput label='Test Field' fieldId='test-id' error={error} register={mockRegister} />
15-117: Good test coverage overall, but consider adding react-hook-form integration tests.The tests comprehensively cover rendering and accessibility, but don't verify that the
registerprops (onChange,onBlur,ref) are correctly passed to the underlying input element. This could catch integration issues with react-hook-form.♻️ Optional test to verify register integration
it('should pass register props to input element', () => { render( <FormFieldInput label='Test Field' fieldId='test-id' register={mockRegister} />, { wrapper: defaultAppRoot }, ); const input = screen.getByRole('textbox'); expect(mockRegister.ref).toHaveBeenCalledWith(input); });packages/web-ui-registration/src/components/PasswordFieldWithVerifier.spec.tsx (1)
118-126: Avoidas anyby typing errors withFieldErrorfrom react-hook-form.This keeps the test aligned with library typings and avoids masking type regressions.
♻️ Proposed refactor
import { mockAppRoot } from '@rocket.chat/mock-providers'; import { render, screen } from '@testing-library/react'; +import type { FieldError } from 'react-hook-form'; @@ - const error = { type: 'required', message: 'Password is required' }; + const error: FieldError = { type: 'required', message: 'Password is required' }; @@ - const error = { type: 'validate', message: "Passwords don't match" }; + const error: FieldError = { type: 'validate', message: "Passwords don't match" };Also applies to: 136-147
packages/web-ui-registration/src/hooks/useRegisterFormValidation.spec.tsx (1)
96-105: Remove the inline comment in the test.
It’s redundant with the test name and violates the “no implementation comments” guideline. As per coding guidelines, ...💡 Proposed change
- // With disabled password policy, any password is valid expect(result.current.passwordIsValid).toBe(true);packages/web-ui-registration/src/RegisterForm.spec.tsx (1)
10-10: Remove inline comments in tests.The test names already convey intent; inline comments add noise. As per coding guidelines, avoid code comments in implementation/tests.
🧹 Example cleanup
-// Mock the useRegisterMethod hook ... - // Fill in the form ... - // Submit the form ... - // The CustomFieldsForm is already tested in ui-client - // We just verify it's rendered in RegisterForm ... - // CustomFieldsForm component should be in the document - // This is an integration test to ensure the component is properly includedAlso applies to: 276-283, 358-364
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/web-ui-registration/src/RegisterForm.spec.tsx`:
- Line 21: The variable declaration has an unintended space ("defaultApp Root")
causing all references to defaultAppRoot to be undefined; change the declaration
to match usages by renaming the identifier to defaultAppRoot and assign it the
mock value from mockAppRoot() (i.e., make the declaration const defaultAppRoot =
mockAppRoot()), then run tests to ensure all references (e.g., in the test cases
at lines referencing defaultAppRoot) resolve correctly.
🧹 Nitpick comments (1)
packages/web-ui-registration/src/RegisterForm.spec.tsx (1)
10-13: Drop inline comments to keep tests concise.The comments restate intent already covered by the test names. As per coding guidelines, avoid implementation comments.
♻️ Suggested cleanup
-// Per-test user instance let user: ReturnType<typeof userEvent.setup>; -// Mock the useRegisterMethod hook jest.mock('./hooks/useRegisterMethod', () => ({ useRegisterMethod: () => ({ mutate: mockRegisterMethod, isPending: false, }), })); @@ - // Fill in the form await user.type(screen.getByRole('textbox', { name: /name/i }), 'John Doe'); await user.type(screen.getByRole('textbox', { name: /email/i }), '[email protected]'); await user.type(screen.getByRole('textbox', { name: /username/i }), 'johndoe'); await user.type(screen.getByLabelText(/password/i), 'password123'); - // Submit the form await user.click(screen.getByRole('button', { name: /join your team/i })); @@ - // Mock custom fields to verify CustomFieldsForm integration const mockCustomFields = [ { _id: 'field1', type: 'text', label: 'Custom Field 1', required: true, defaultValue: '', public: true }, ]; @@ - // Verify CustomFieldsForm integration - detailed tests are in ui-client const form = screen.getByRole('form');Also applies to: 280-287, 362-375
|
I’ve addressed the requested changes and fixed the follow-ups. Re-requesting review when you have a moment, thanks! |
2 participants
fixes #38364
What was the problem?
RegisterForm.tsxhad grown significantly complex and required disabling the ESLint complexity rule:/* eslint-disable complexity */The file (~316 lines) mixed multiple responsibilities:
This made the component harder to read, maintain, and safely evolve.
What does this PR do?
This PR performs a pure refactor to reduce complexity without changing any behavior or UI.
Key changes
Removed
eslint-disable complexityExtracted validation logic into a custom hook
useRegisterFormValidationreact-hook-formbehaviorExtracted registration error handling
useRegisterErrorHandlerReduced repetitive JSX
FormFieldInputfor common field patternsPasswordFieldWithVerifierfor password + verifier + confirmation logicaria-*) attributes preservedSimplified
RegisterForm.tsxWhat this PR does NOT do
This is strictly a refactor.
Why this approach?
LoginForm)Files changed
Modified
packages/web-ui-registration/src/RegisterForm.tsxAdded
hooks/useRegisterFormValidation.tshooks/useRegisterErrorHandler.tscomponents/FormFieldInput.tsxcomponents/PasswordFieldWithVerifier.tsxSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.