fix: User admin GUI crash when rendering an invalid custom field type #37154
Conversation
🦋 Changeset detectedLatest commit: 77f158b The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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 |
WalkthroughAdds a dependency patch changeset and release note for an admin GUI crash. Updates CustomFieldsForm to safely skip invalid custom field types and refine label/ARIA handling. Introduces component unit tests and an end-to-end test to verify that invalid custom fields are not rendered while valid ones are shown. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin as Admin User
participant UI as Admin UI (Users)
participant Form as CustomFieldsForm
participant Meta as Custom Fields Metadata
Admin->>UI: Open user edit form
UI->>Meta: Load custom fields metadata
Meta-->>UI: Field definitions (types, labels, rules)
UI->>Form: Render with metadata
loop For each field
Form->>Form: Resolve field component by type
alt Valid type
Form->>UI: Render input with label/id and ARIA
note right of Form: aria-labelledby set to label id<br/>aria-describedby set only on error
else Invalid type
Form->>Form: Skip rendering (no throw)
note right of Form: Guard: render nothing
end
end
Admin->>UI: Interact/validate
UI-->>Admin: Form renders without crash
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37154 +/- ##
===========================================
- Coverage 67.39% 67.38% -0.01%
===========================================
Files 3286 3287 +1
Lines 111584 111684 +100
Branches 20377 20408 +31
===========================================
+ Hits 75197 75256 +59
- Misses 33695 33739 +44
+ Partials 2692 2689 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
aleksandernsilva
changed the title
Fixes a GUI crash happening in the admin user page when attempting to display an invalid custom field
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/tests/e2e/admin-users-custom-fields.spec.ts (1)
145-161: Isolate the invalid custom field test by using a fresh user
The test inwith invalid custom field type(lines 145–161) reusesaddTestUsershared with earlier tests, risking cross‐test state leakage; wrap it in a localbeforeAll/afterAllto create and delete its own test user.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/beige-planets-suffer.md(1 hunks)apps/meteor/tests/e2e/admin-users-custom-fields.spec.ts(1 hunks)packages/ui-client/src/components/CustomFieldsForm.spec.tsx(1 hunks)packages/ui-client/src/components/CustomFieldsForm.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Use descriptive test names that clearly communicate expected behavior
Use test.beforeAll() and test.afterAll() for setup and teardown
Use test.step() to organize complex test scenarios
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) consistently
Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Maintain test isolation between test cases
Ensure a clean state for each test execution
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/admin-users-custom-fields.spec.ts
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/admin-users-custom-fields.spec.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/admin-users-custom-fields.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/tests/e2e/admin-users-custom-fields.spec.ts
🧬 Code graph analysis (1)
packages/ui-client/src/components/CustomFieldsForm.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)packages/ui-client/src/components/CustomFieldsForm.tsx (1)
CustomFieldsForm(105-111)
🔇 Additional comments (8)
.changeset/beige-planets-suffer.md (1)
1-6: LGTM!The changeset follows the correct format and clearly describes the GUI crash fix for invalid custom field types.
packages/ui-client/src/components/CustomFieldsForm.tsx (5)
67-69: LGTM!The guard prevents rendering and subsequent crashes when an invalid field type is encountered. This is the core fix for the GUI crash issue.
79-81: LGTM!Using an explicit
idon the label (rendered as a span) allows proper ARIA association viaaria-labelledbyon Line 86. This is a valid accessibility pattern when the label is not a clickable<label>element.
86-87: LGTM!The ARIA attributes correctly associate the input with its label and conditionally with error messaging. The
aria-describedbyonly includes the error ID when an error message exists, preventing invalid references to non-existent elements.
93-97: LGTM!Conditionally rendering the error only when
errorMessageexists prevents empty error containers in the DOM and aligns with the conditionalaria-describedbyon Line 87.
107-109: LGTM!Deriving the label from metadata and providing a fallback to
fieldNameis cleaner than mutating props inline. This change simplifies the label handling logic.packages/ui-client/src/components/CustomFieldsForm.spec.tsx (2)
1-31: LGTM!The test setup properly wraps the component with react-hook-form and provides translation mocking via
mockAppRoot. TheTestComponentwrapper follows best practices for testing form-controlled components.
128-138: LGTM!This test validates the critical fix: invalid field types do not crash the component and are not rendered. The test uses
expect().not.toThrow()to verify no crash occurs andqueryByLabelText().not.toBeInTheDocument()to confirm the field is not rendered.
tassoevan
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Fixes an issue causing the GUI to crash when editing users with custom fields.
The crash occurred when a custom field had an invalid type. The React component responsible for rendering these fields failed to handle invalid types, leading to a rendering error.
Solution
Issue(s)
CORE-1313
Steps to test or reproduce
Further comments
A separate PR refactoring the
CustomFieldFormwill be opened with more improvements, including recent best practices, error boundary, etc.Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores