refactor(menu): replace any-typed ref with forwardRef #38295
Conversation
fix(admin): make Custom HTML card responsive on mobile
…n-loader fix(ui): add skeleton loading states to Room Contextual Bar views
…ptions fix: prevent startup crash on invalid MONGO_OPTIONS JSON
|
|
|
|
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 and enhances multiple components across the codebase: adds defensive logic to sidebar item deregistration, introduces a new skeleton loading component for contextual lists, refactors MongoDB configuration and email handling, restructures the Menu component to use forwardRef with typed sub-components, and consolidates BaseRaw model methods with enhanced trash rollback support. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/packages/rocketchat-mongo-config/server/index.js (1)
31-38: Validate MONGO_OPTIONS is a plain object before merging.
If the JSON parses tonull, an array, or a primitive, the merge either fails or yields unexpected keys. Please guard and emit a clearer error for non-object values.🛠️ Proposed fix
- try { - const mongoOptions = JSON.parse(mongoOptionStr); - Object.assign(mongoConnectionOptions, mongoOptions); - } catch (error) { - throw new Error('Invalid MONGO_OPTIONS environment variable: must be valid JSON.', { cause: error }); - } + let mongoOptions; + try { + mongoOptions = JSON.parse(mongoOptionStr); + } catch (error) { + throw new Error('Invalid MONGO_OPTIONS environment variable: must be valid JSON.', { cause: error }); + } + + if (mongoOptions === null || typeof mongoOptions !== 'object' || Array.isArray(mongoOptions)) { + throw new Error('Invalid MONGO_OPTIONS environment variable: must be a JSON object.'); + } + Object.assign(mongoConnectionOptions, mongoOptions);
🧹 Nitpick comments (4)
packages/models/src/models/BaseRaw.ts (2)
170-170: Remove marker comments.These
/* ===================== FIXED METHOD ===================== */style comments are debug/development artifacts that add noise without providing meaningful documentation. As per coding guidelines, avoid code comments in the implementation.Proposed fix
- /* ===================== FIXED METHOD ===================== */ - async findOneAndDelete(filter: Filter<T>, options?: FindOneAndDeleteOptions): Promise<WithId<T> | null> {- /* ======================================================== */ - private setUpdatedAt(record: UpdateFilter<T> | InsertionModel<T>): void {Also applies to: 222-222
157-168:any-typed options parameter undermines type safety.Both
findOneByIdandfindOneimplementation signatures useoptions?: any, which defeats TypeScript's type checking for MongoDBFindOptions. Consider using proper union types or a constrained generic to preserve type safety.Suggested approach
- async findOneById(_id: T['_id'], options?: any): Promise<T | null> { + async findOneById(_id: T['_id'], options?: FindOptions<T>): Promise<T | null> { const query: Filter<T> = { _id } as Filter<T>; return options ? this.findOne(query, options) : this.findOne(query); } - async findOne<P extends Document = T>(query: Filter<T> | T['_id'] = {}, options?: any): Promise<WithId<T> | WithId<P> | null> { + async findOne<P extends Document = T>(query: Filter<T> | T['_id'] = {}, options?: FindOptions<T> | FindOptions<P>): Promise<WithId<T> | WithId<P> | null> {apps/meteor/packages/rocketchat-mongo-config/server/index.js (1)
15-18: Avoid inline TODO/FIX comments in implementation.
Please move the TODO/FIX note to the issue tracker and keep the code comment-free to match the project’s style expectations. As per coding guidelines, ...packages/livechat/src/components/Menu/index.tsx (1)
10-13: Consider removing the section divider comments.
These banner comments add noise, and the local guidelines prefer comment-free implementation blocks.As per coding guidelines, avoid implementation comments.🧹 Suggested cleanup
-/* ------------------------------------------------------------------ */ -/* Menu */ -/* ------------------------------------------------------------------ */ - -/* ------------------------------------------------------------------ */ -/* Group */ -/* ------------------------------------------------------------------ */ - -/* ------------------------------------------------------------------ */ -/* Item */ -/* ------------------------------------------------------------------ */ - -/* ------------------------------------------------------------------ */ -/* PopoverMenuWrapper */ -/* ------------------------------------------------------------------ */ - -/* ------------------------------------------------------------------ */ -/* PopoverMenu */ -/* ------------------------------------------------------------------ */ - -/* ------------------------------------------------------------------ */ -/* Static bindings */ -/* ------------------------------------------------------------------ */Also applies to: 33-36, 48-51, 77-80, 156-159, 185-187
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
2 participants
Proposed changes (including videos or screenshots)
This PR refactors the
Menucomponent to improve ref handling by removing anany-typed ref and an explicitFIXMEcomment.The component is updated to use a properly typed
forwardRef<HTMLDivElement>, ensuring:This is a type-only refactor and does not introduce any UI or behavioral changes. Existing subcomponents (
Menu.Group,Menu.Item,Menu.Popover) continue to work as before.Issue(s)
Fixes #38273
Steps to test or reproduce
Menucomponent anywhere it is currently rendered.Menunow resolve to the underlyingHTMLDivElement.Further comments
This change addresses acknowledged technical debt by removing an
any-typed ref and aligning the component with standard Preact ref patterns. The refactor is intentionally minimal and scoped to improve type safety without affecting runtime behavior.Note:
A local build was attempted on Windows; it failed in
@rocket.chat/favicondue to the use of the Unix-onlyrmcommand. This failure is environment-specific and unrelated to the changes in this PR. CI is expected to validate the update successfully.GitHub currently reports that this branch cannot be automatically merged due to parallel changes in the same area. The branch is rebased on the latest
develop, and final conflict resolution can be handled during merge if required.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.