feat: add admin info step to setup wizard with dynamic imports #37853
Conversation
- Refine useHasLicenseModule to use dynamic imports and proper state - Implement AdminInfoStep component for setup wizard with form validation - Add necessary imports and types
|
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 |
|
WalkthroughThis PR refactors client-side module loading to use dynamic imports of Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/meteor/client/startup/streamMessage/autotranslate.ts (1)
11-24: Consider adding error handling for dynamic imports.Both dynamic import paths lack
.catch()handlers. If the import fails, the callback removal/addition silently fails, which could lead to:
- Stale callbacks remaining registered when AutoTranslate is disabled
- Missing callbacks when enabled, causing autotranslate to not work
if (!isEnabled) { import('@rocket.chat/ui-client').then(({ clientCallbacks }) => { clientCallbacks.remove('streamMessage', 'autotranslate-stream'); - }); + }).catch((error) => { + console.error('Failed to load ui-client for autotranslate cleanup:', error); + }); return; } import('../../../app/autotranslate/client').then(({ createAutoTranslateMessageStreamHandler }) => { const streamMessage = createAutoTranslateMessageStreamHandler(); import('@rocket.chat/ui-client').then(({ clientCallbacks }) => { clientCallbacks.remove('streamMessage', 'autotranslate-stream'); clientCallbacks.add('streamMessage', streamMessage, clientCallbacks.priority.HIGH - 3, 'autotranslate-stream'); + }).catch((error) => { + console.error('Failed to load ui-client for autotranslate:', error); }); });apps/meteor/client/views/setupWizard/steps/AdminInfoStep.tsx (2)
35-50: Consider handling post-registration navigation or form reset.After successful admin user creation, the form remains in its current state. Users may be confused about next steps.
Consider:
- Resetting the form after success
- Navigating to the next wizard step
- Showing a more prominent success state
+ const { reset } = useForm<AdminInfoFormData>(); + const { mutate: registerAdminUser, isPending } = useMutation({ mutationFn: async (data: AdminInfoFormData) => { await registerUser({ name: data.name, username: data.username, email: data.email, pass: data.password, }); }, onSuccess: () => { dispatchToastMessage({ type: 'success', message: t('Admin_user_created_successfully' as any) }); + reset(); + // TODO: Navigate to next step if applicable },
75-80: Redundant password validation function.The
validatePasswordfunction only checks for empty/missing password, which can be handled by therequiredrule directly. This simplifies the validation logic.<Controller control={control} name='password' - rules={{ validate: validatePassword }} + rules={{ required: t('Required_field', { field: t('Password') }) }} render={({ field }) => (This also allows removing the
validatePasswordfunction entirely.
📜 Review details
Configuration used: Organization 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 (5)
apps/meteor/client/hooks/useHasLicenseModule.ts(1 hunks)apps/meteor/client/startup/routes.tsx(1 hunks)apps/meteor/client/startup/streamMessage/autotranslate.ts(1 hunks)apps/meteor/client/views/root/AppErrorPage.tsx(1 hunks)apps/meteor/client/views/setupWizard/steps/AdminInfoStep.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/root/AppErrorPage.tsxapps/meteor/client/views/setupWizard/steps/AdminInfoStep.tsxapps/meteor/client/startup/streamMessage/autotranslate.tsapps/meteor/client/startup/routes.tsxapps/meteor/client/hooks/useHasLicenseModule.ts
🧠 Learnings (3)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/root/AppErrorPage.tsx
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
apps/meteor/client/startup/streamMessage/autotranslate.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/client/startup/streamMessage/autotranslate.ts
🧬 Code graph analysis (3)
apps/meteor/client/views/setupWizard/steps/AdminInfoStep.tsx (1)
packages/ui-contexts/src/index.ts (4)
useTranslation(81-81)useToastMessageDispatch(78-78)useSetting(72-72)useMethod(55-55)
apps/meteor/client/startup/streamMessage/autotranslate.ts (1)
apps/meteor/app/autotranslate/client/lib/autotranslate.ts (1)
createAutoTranslateMessageStreamHandler(125-162)
apps/meteor/client/hooks/useHasLicenseModule.ts (1)
packages/core-typings/src/license/LicenseModule.ts (1)
LicenseModule(30-30)
🔇 Additional comments (2)
apps/meteor/client/views/root/AppErrorPage.tsx (1)
17-17: LGTM!The background color change to
'surface-light'aligns with the PR objective and provides better visual consistency.apps/meteor/client/startup/routes.tsx (1)
202-205: LGTM!Wrapping
SetupWizardRoutewithappLayout.wrap()provides consistent layout context, matching the pattern used by other routes in this file.
| export const useHasLicenseModule = (_licenseName: LicenseModule | undefined) => { | ||
| const [result, setResult] = useState({ data: false, isPending: true, isError: false }); | ||
|
|
||
| useEffect(() => { | ||
| import('@rocket.chat/ui-client') | ||
| .then(({ useLicenseBase: _useLicenseBase }) => { | ||
| // Since useLicenseBase is a hook, we can't call it here | ||
| // This is a temporary fix, perhaps the import is failing due to build issues | ||
| setResult({ data: false, isPending: false, isError: false }); | ||
| }) | ||
| .catch(() => { | ||
| setResult({ data: false, isPending: false, isError: true }); | ||
| }); | ||
| }, []); | ||
|
|
||
| return result; | ||
| }; |
There was a problem hiding this comment.
Critical: License module check is completely broken.
This implementation always returns data: false, effectively disabling all license-gated features. The approach is fundamentally flawed:
- React hooks cannot be called inside callbacks -
useLicenseBaseis imported but assigned to_useLicenseBase(unused) because hooks can't be called in async/useEffect callbacks _licenseNameparameter is never used - The license module is never checked- Always returns false - Line 13 hardcodes
data: falseregardless of actual license state
This will break all features that depend on license module checks (e.g., enterprise features).
The dynamic import pattern cannot work with React hooks. Consider alternative approaches:
- Lazy component wrapper: Wrap license-dependent components with a dynamically-imported component that internally uses the hook
- Context provider at app root: Load the license hook at app initialization and provide it via context
- Investigate the original build issue: The comment suggests this is a workaround for build issues - fixing the root cause may be preferable
How to dynamically import React hooks in a component?
| const regexpForUsernameValidation = useSetting('UTF8_User_Names_Validation'); | ||
| const usernameRegExp = new RegExp(`^${regexpForUsernameValidation}$`); | ||
| const usernameBlackList = ['all', 'here', 'admin'].map((username) => new RegExp(`^${username.trim()}$`, 'i')); | ||
| const hasBlockedName = (username: string): boolean => | ||
| !!usernameBlackList.length && usernameBlackList.some((restrictedUsername) => restrictedUsername.test(username.trim())); |
There was a problem hiding this comment.
Handle undefined setting value for username regex.
If UTF8_User_Names_Validation setting returns undefined or an empty string, new RegExp() will create a regex that matches empty strings, causing unexpected validation behavior.
const regexpForUsernameValidation = useSetting('UTF8_User_Names_Validation');
- const usernameRegExp = new RegExp(`^${regexpForUsernameValidation}$`);
+ const usernameRegExp = regexpForUsernameValidation ? new RegExp(`^${regexpForUsernameValidation}$`) : null;
const usernameBlackList = ['all', 'here', 'admin'].map((username) => new RegExp(`^${username.trim()}$`, 'i'));
const hasBlockedName = (username: string): boolean =>
!!usernameBlackList.length && usernameBlackList.some((restrictedUsername) => restrictedUsername.test(username.trim()));Then update validateUsername to handle the null case:
const validateUsername = (username: string): boolean | string => {
- if (!usernameRegExp.test(username) || hasBlockedName(username)) {
+ if ((usernameRegExp && !usernameRegExp.test(username)) || hasBlockedName(username)) {
return t('Invalid_username');
}
return true;
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/meteor/client/views/setupWizard/steps/AdminInfoStep.tsx around lines 27
to 31, the code constructs a RegExp from the UTF8_User_Names_Validation setting
without guarding against undefined or empty values which causes a regex that
matches empty strings; change it to first read the setting into a local
variable, check if it is a non-empty string and only then construct
usernameRegExp (otherwise set it to null or a safe default that enforces a
reasonable rule), keep usernameBlackList creation unchanged, and update
validateUsername to treat a null/absent usernameRegExp as "no custom regex"
(skip that test) so validation behaves correctly when the setting is missing or
blank.
| onSuccess: () => { | ||
| dispatchToastMessage({ type: 'success', message: t('Admin_user_created_successfully' as any) }); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the translation keys in i18n files
rg -n "Admin_user_created_successfully|Create_admin_user" --type jsonRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Read the AdminInfoStep.tsx file to see lines around the translation key usage
wc -l apps/meteor/client/views/setupWizard/steps/AdminInfoStep.tsxRepository: RocketChat/Rocket.Chat
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Read the specific lines mentioned in the review
echo "=== Lines 44-46 ==="
sed -n '44,46p' apps/meteor/client/views/setupWizard/steps/AdminInfoStep.tsx
echo -e "\n=== Lines 235-245 ==="
sed -n '235,245p' apps/meteor/client/views/setupWizard/steps/AdminInfoStep.tsx
echo -e "\n=== Check imports and context around line 44 ==="
sed -n '1,50p' apps/meteor/client/views/setupWizard/steps/AdminInfoStep.tsxRepository: RocketChat/Rocket.Chat
Length of output: 2339
🏁 Script executed:
#!/bin/bash
# Check if there's an i18n file in the expected location
ls -la packages/i18n/src/locales/en.i18n.json 2>/dev/null || echo "File not found"
# Search for similar keys with different naming patterns
echo -e "\n=== Search for similar admin user keys ==="
rg -i "admin.*user|user.*created" --type json | head -20
# Search for any Create_admin patterns
echo -e "\n=== Search for Create_admin patterns ==="
rg "Create_admin" --type json
# Search for Admin_user patterns
echo -e "\n=== Search for Admin_user patterns ==="
rg "Admin_user" --type jsonRepository: RocketChat/Rocket.Chat
Length of output: 3443
🏁 Script executed:
#!/bin/bash
# Search specifically in the English i18n file for admin-related keys or user creation keys
echo "=== Search for 'User_created' in en.i18n.json ==="
rg "User_created" packages/i18n/src/locales/en.i18n.json
echo -e "\n=== Search for 'admin' keys (case-insensitive) in en.i18n.json ==="
rg -i "admin.*form|onboarding.*admin" packages/i18n/src/locales/en.i18n.json
echo -e "\n=== Search for 'Create' keys in en.i18n.json ==="
rg "Create_" packages/i18n/src/locales/en.i18n.json | head -20
echo -e "\n=== Check the size and sample of en.i18n.json ==="
head -100 packages/i18n/src/locales/en.i18n.json | tail -50Repository: RocketChat/Rocket.Chat
Length of output: 7015
🏁 Script executed:
#!/bin/bash
# Double-check by searching for the exact keys with different patterns
echo "=== Exact search in en.i18n.json for Admin_user_created_successfully ==="
grep -n "Admin_user_created_successfully" packages/i18n/src/locales/en.i18n.json || echo "NOT FOUND"
echo -e "\n=== Exact search in en.i18n.json for Create_admin_user ==="
grep -n "Create_admin_user" packages/i18n/src/locales/en.i18n.json || echo "NOT FOUND"
echo -e "\n=== Check if there's a pattern like these keys elsewhere ==="
rg "onboarding.*Create|onboarding.*success" packages/i18n/src/locales/en.i18n.jsonRepository: RocketChat/Rocket.Chat
Length of output: 557
Add missing translation keys for admin user creation.
The translation keys 'Admin_user_created_successfully' and 'Create_admin_user' do not exist in the i18n definitions. The as any casts on lines 45 and 240 are masking this issue. Add these keys to packages/i18n/src/locales/en.i18n.json and remove the type casts.
🤖 Prompt for AI Agents
In apps/meteor/client/views/setupWizard/steps/AdminInfoStep.tsx around lines
44-46 (and also remove the 'as any' cast at line ~240), the code uses
translation keys 'Admin_user_created_successfully' and 'Create_admin_user' that
don't exist and are being hidden by 'as any' casts; add these two keys with
appropriate English strings to packages/i18n/src/locales/en.i18n.json (e.g.
"Admin_user_created_successfully": "Admin user created successfully",
"Create_admin_user": "Create admin user"), then remove the 'as any' casts in
AdminInfoStep.tsx so the t(...) calls use the correct typed keys and
compile-time checks will catch missing translations.
mandar1045
changed the title
mandar1045
left a comment
mandar1045
left a comment
There was a problem hiding this comment.
This PR includes several improvements to the client-side codebase:
useHasLicenseModule.ts: Replaced direct useLicenseBase hook usage with a custom implementation using useState and useEffect, added dynamic import of @rocket.chat/ui-client for better build reliability, and enhanced error handling with isLoading and isError properties.
routes.tsx: Wrapped SetupWizardRoute component with appLayout.wrap() to ensure proper layout rendering.
autotranslate.ts: Changed direct import of clientCallbacks to dynamic imports to prevent module loading issues.
AppErrorPage.tsx: Updated background color from 'surface' to 'surface-light' for improved visual consistency in error states.
AdminInfoStep.tsx: Added new empty file for the Admin Info step in the setup wizard (to be populated in future commits).
These changes focus on enhancing module loading reliability, fixing layout issues, and preparing for setup wizard enhancements.
mandar1045
changed the title
mandar1045
changed the title
|
Hey, it is not clear what exactly this PR is trying to achieve, and the changes to the setup wizard are not wanted. I'll be closing this PR. I would recommend checking existing issues and taking a look at our documentation before sending actual code. |
|
thank you for your feedback ! @MartinSchoeler , Appreciate you pointing these out - I'll fix them and look forward to existing issues. |
3 participants
This PR addresses build issues by implementing dynamic imports for the license hook and adds a new admin info step component to the setup wizard.
Changes:
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements
✏️ Tip: You can customize this high-level summary in your review settings.