feat: enhance performance with memoization and error logging in vario…#38631
feat: enhance performance with memoization and error logging in vario…#38631IvanNilash1206 wants to merge 4 commits into
Conversation
|
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 React.memo to several avatar components; tightens TypeScript signatures (replacing Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client
participant BaseRaw as BaseRaw.findOneById()
participant Cache as In-Memory Cache
participant DB as MongoDB
Caller->>BaseRaw: findOneById(id, options?)
alt Cache enabled and no options
BaseRaw->>Cache: getCached(id)
alt Cache hit (not expired)
Cache-->>BaseRaw: cached document
BaseRaw-->>Caller: return cached document
else Cache miss or expired
BaseRaw->>DB: query by id
DB-->>BaseRaw: document
BaseRaw->>Cache: setCached(id, document)
BaseRaw-->>Caller: return document
end
else Cache disabled or options provided
BaseRaw->>DB: query by id
DB-->>BaseRaw: document
BaseRaw-->>Caller: return document
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/models/src/models/BaseRaw.ts (1)
242-260:⚠️ Potential issue | 🔴 CriticalCache invalidation is incomplete and fragile for non-
_idfilters.Two issues:
Filter assumption:
invalidateCacheis only called when'_id' in filter, but updates can match documents by other fields. Also,filter._idcould be an operator ({ $in: [...] }) rather than a plain string — casting it tostringwould produce a nonsensical key. InfindOneAndUpdate,result._idis available and should be used instead.Missing invalidation on delete paths:
deleteOne,deleteMany,findOneAndDelete,removeById, andremoveByIdsnever invalidate the cache. A deleted document will continue to be served from cache until TTL expires. Similarly,updateManyskips invalidation entirely.This will cause stale/phantom reads for any model that opts into caching.
🐛 Sketch: safer invalidation in findOneAndUpdate
const result = await this.col.findOneAndUpdate(query, update, options || {}); - if (result && '_id' in query) { - this.invalidateCache(query._id as string); - } + if (result) { + this.invalidateCache(result._id as unknown as string); + } return result;For delete methods, add
invalidateCachecalls indeleteOne,findOneAndDelete,deleteMany, andremoveById/removeByIds(or clear the entire cache on bulk deletes).Also applies to: 338-360
🤖 Fix all issues with AI agents
In
`@apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarSuggestions.tsx`:
- Around line 12-13: Remove the inline comment immediately above the
UserAvatarSuggestions declaration (the "// Memoized to prevent re-renders when
parent updates but props haven't changed" line) so the function declaration
const UserAvatarSuggestions = memo(function UserAvatarSuggestions({ disabled,
onSelectOne }: UserAvatarSuggestionsProps) { remains uncommented; ensure no
other implementation comments remain in this file around the
UserAvatarSuggestions function to comply with the project's comment guidelines.
In
`@apps/meteor/client/components/message/content/attachments/structure/AttachmentAuthorAvatar.tsx`:
- Around line 5-6: Remove the inline comment before the AttachmentAuthorAvatar
declaration; leave the memoized component intact (AttachmentAuthorAvatar =
memo(({ url }: { url: string }): ReactElement => <BaseAvatar url={url}
size='x24' />)); simply delete the comment text "// Memoized to prevent
re-renders when parent updates but URL hasn't changed" to comply with the coding
guideline prohibiting implementation comments.
In `@apps/meteor/client/components/UserAutoCompleteMultiple/UserAvatarChip.tsx`:
- Around line 13-14: Remove the inline implementation comment preceding the
UserAvatarChip declaration: delete the comment "// Memoized to prevent
re-renders in lists when sibling items change" so the component definition const
UserAvatarChip = memo(({ federated, username, name, ...props }:
UserAvatarChipProps) => { remains without inline comments; leave the memo
wrapper and UserAvatarChipProps typing unchanged.
In `@apps/meteor/client/components/UserInfo/UserInfoAvatar.tsx`:
- Around line 5-6: Remove the inline implementation comment above the memoized
component; leave the memoized export intact. Specifically, edit the
UserInfoAvatar declaration (the memo wrapper around UserAvatar using
ComponentProps<typeof UserAvatar>) and delete the leading comment line ("//
Memoized to prevent re-renders when parent component updates") so the file
contains only the existing const UserInfoAvatar = memo((props:
ComponentProps<typeof UserAvatar>): ReactElement => <UserAvatar size='x332'
{...props} />); without any implementation comments.
In `@packages/models/src/models/BaseRaw.ts`:
- Around line 47-55: Remove all inline and block comments introduced in
BaseRaw.ts to comply with the repository guideline "Avoid code comments in the
implementation"; specifically delete the comment blocks around the cache
property/type declarations and the CacheEntry<T> type and any other // or /* */
comments present (search for occurrences near symbols like cache, CacheEntry,
findOneById and other functions in BaseRaw). After removing the comments, ensure
no commented-out code is needed for logic, run the TypeScript build/tests to
confirm nothing was accidentally removed that affects compilation, and keep only
executable code and JSDoc-free signatures.
- Around line 170-181: The forEach callback in BaseRaw.invalidateCache currently
has an expression body that implicitly returns the boolean from Map.delete,
triggering the lint warning; update the callback to use a block body that
discards the return value (e.g., change id.forEach((i) => this.cache?.delete(i))
to id.forEach((i) => { this.cache?.delete(i); }) ) or replace the forEach with
an equivalent for...of loop to ensure the callback does not return a value; keep
changes inside the invalidateCache method.
- Around line 69-73: The in-memory cache fields on BaseRaw (cache, cacheTTL,
cacheMaxSize) create per-process caches that can serve stale reads in
multi-instance deployments; update the ModelOptions type and BaseRaw
constructor/docs to explicitly warn that cache is per-process and not
invalidated across instances, recommend limiting cache enablement to
eventually-consistent, read-heavy models (e.g., not permissions/roles), and add
a comment or runtime warning when a model is created with caching enabled
suggesting a shared cache (Redis) for strong consistency; reference the BaseRaw
class and the options that enable caching (cache, cacheTTL, cacheMaxSize,
ModelOptions) so callers can make an informed choice.
- Around line 154-168: The current eviction in setCached implements FIFO because
Map preserves insertion order and getCached does not update entry order; change
getCached to perform a delete-and-reinsert on every cache hit (e.g., read the
entry, delete the key, then set it again) so the accessed key becomes
most-recently-used, and also update setCached to delete existing key before
setting to ensure updates move the key to the end; reference the methods
getCached and setCached when making these changes.
In `@packages/models/src/models/Rooms.ts`:
- Around line 99-108: Remove the inline trailing comments in the Rooms model
index definitions and drop the deprecated background: true option from the
compound index objects; specifically edit the index entries that use key: {
teamId: 1, archived: 1 } and key: { t: 1, archived: 1 } to remove any inline
comments and remove the background: true property (leave other properties like
sparse if still desired) so the index definitions conform to MongoDB driver
6.10.0 and the project's no-inline-comment guideline.
In `@packages/models/src/models/Subscriptions.ts`:
- Around line 1664-1685: The return assembly in unsetBlockedByRoomId is using
legacy BulkWriteResult properties (result.nMatched/result.nModified); update the
code to use the modern driver properties (result.matchedCount and
result.modifiedCount) when constructing each UpdateResult, mirroring the fix
applied to setBlockedByRoomId and keeping the same shape (acknowledged,
matchedCount, modifiedCount, upsertedCount, upsertedId) for both returned
entries.
- Around line 1640-1661: setBlockedByRoomId currently reads non-existent
BulkWriteResult props (nMatched/nModified) and returns identical aggregate
counts for both operations; replace those with the correct properties
(result.matchedCount and result.modifiedCount) if you accept aggregate totals,
OR better: perform two separate updateOne calls (via Promise.all) to get
per-operation UpdateResult objects and return them so callers in
blockUser.ts/unblockUser.ts can inspect modifiedCount per operation; update the
function setBlockedByRoomId to either 1) change the returned fields to use
result.matchedCount/result.modifiedCount (and keep bulkWrite) or 2) replace the
bulkWrite call with two await this.col.updateOne calls wrapped in Promise.all
and return their individual UpdateResult values.
In `@packages/ui-voip/src/context/MediaCallContext.ts`:
- Around line 119-120: Remove the inline comment on the onToggleWidget
declaration so the file uses the PeerInfo type without an implementation
comment; specifically update the MediaCallContext.ts declaration for
onToggleWidget (currently typed as onToggleWidget: (peerInfo?: PeerInfo) =>
void) by deleting the preceding comment line, keeping consistency with
MediaCallContextType and MediaCallExternalContextType.
In `@packages/ui-voip/src/context/MediaCallLogger.ts`:
- Line 11: Remove the inline implementation comment at the top of
MediaCallLogger.ts (the comment explaining the reason for replacing any[] with
unknown[]); delete that comment line(s) and leave the type change intact (e.g.,
where any[] was updated to unknown[]) so MediaCallLogger and its exported
functions/classes contain no implementation-level comments per the coding
guidelines.
- Around line 12-26: The MediaCallLogger implementation uses parameter types
unknown[] for log/debug/error/warn which is narrower than the IMediaSignalLogger
interface's any[]; update the method signatures on class MediaCallLogger
(methods log, debug, error, warn) to accept any[] instead of unknown[] so the
class correctly implements the IMediaSignalLogger contract, adjusting all four
method declarations accordingly.
🧹 Nitpick comments (7)
packages/ui-voip/src/context/MockedMediaCallProvider.tsx (1)
52-64: Remove the added code comments — the underscore-prefixed params and empty bodies are self-explanatory.The
_deviceprefix and empty function body already signal an intentional no-op; the comments on lines 53, 57, and 63 are redundant.♻️ Suggested diff
- const onDeviceChange = (_device: any) => { - // Device change handled by mocked provider - no logging needed - }; + const onDeviceChange = (_device: any) => {}; const onForward = () => { - // Forward call handled by mocked provider clearState(); setWidgetState('closed'); }; - const onTone = (_tone: string) => { - // Tone input handled by mocked provider - }; + const onTone = (_tone: string) => {};As per coding guidelines,
**/*.{ts,tsx,js}: "Avoid code comments in the implementation".packages/apps-engine/src/server/AppManager.ts (1)
684-687: Inline comment violates coding guidelines.Line 684 adds an explanatory comment. The coding guideline for
**/*.{ts,tsx,js}states to avoid code comments in the implementation. Consider removing it — the.catch+ log pattern is self-explanatory.Also,
erroris untyped here. Since this PR aims to improve type safety, consider typing the catch parameter and guarding.messageaccess:Proposed fix
- // Non-critical notification - log error but don't fail removal await this.bridges.getAppActivationBridge().doAppRemoved(app).catch((error) => { - console.error('Failed to notify app removal', { appId: id, error: error.message }); + console.error('Failed to notify app removal', { appId: id, error: error instanceof Error ? error.message : error }); });As per coding guidelines:
**/*.{ts,tsx,js}: "Avoid code comments in the implementation".apps/meteor/app/livechat/server/startup.ts (1)
136-139: Good use of structured logger; remove inline comment per guidelines.Using
logger.errorinstead of a silent.catch()is a solid improvement. However, line 136 adds an inline comment that should be removed per coding guidelines.Proposed fix
- // Update agent status on logout - log error but don't block logout void setUserStatusLivechatIf(user._id, ILivechatAgentStatus.NOT_AVAILABLE, {}, { livechatStatusSystemModified: true }).catch((error) => { logger.error('Failed to update livechat agent status on logout', { userId: user._id, error }); });As per coding guidelines:
**/*.{ts,tsx,js}: "Avoid code comments in the implementation".ee/apps/ddp-streamer/src/Client.ts (1)
149-160: Remove comments; add type safety to catch parameters consistent with line 82.Both
callMethodandcallSubscribehave inline comments (lines 150, 157) that should be removed per guidelines. Also, theerrorparameter in.catchis untyped and.messageis accessed unsafely — inconsistent with line 82 whereerr: unknownis used.Proposed fix
async callMethod(packet: IPacket): Promise<void> { - // Chain method calls - log errors but continue processing - this.chain = this.chain.then(() => server.call(this, packet)).catch((error) => { - console.error('DDP method call failed', { method: packet.method, error: error.message }); + this.chain = this.chain.then(() => server.call(this, packet)).catch((error: unknown) => { + console.error('DDP method call failed', { method: packet.method, error: error instanceof Error ? error.message : error }); }); } async callSubscribe(packet: IPacket): Promise<void> { - // Chain subscription calls - log errors but continue processing - this.chain = this.chain.then(() => server.subscribe(this, packet)).catch((error) => { - console.error('DDP subscription failed', { name: packet.name, error: error.message }); + this.chain = this.chain.then(() => server.subscribe(this, packet)).catch((error: unknown) => { + console.error('DDP subscription failed', { name: packet.name, error: error instanceof Error ? error.message : error }); }); }As per coding guidelines:
**/*.{ts,tsx,js}: "Avoid code comments in the implementation".apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarSuggestions.tsx (1)
13-15:memohas limited benefit here; the type cast erodes safety.Two observations:
Since
useUserAvatarSuggestions()is called inside this component, any hook state change (loading → loaded) triggers a re-render regardless ofmemo. The memoization only guards against parent-driven re-renders with unchangeddisabled/onSelectOneprops — make sure that's the scenario you're optimizing for.The
as UserAvatarSuggestion[]cast on line 15 silently discards type checking. Consider narrowing the hook's return type instead so the cast isn't needed.packages/models/src/models/Subscriptions.ts (1)
70-79: Remove inline comments;background: trueis a no-op since MongoDB 4.2.Same concern as in
Rooms.ts—the coding guideline for**/*.{ts,tsx,js}says to avoid code comments in the implementation. Thebackgroundoption is also ignored since MongoDB 4.2.As per coding guidelines, "Avoid code comments in the implementation."
packages/models/src/models/Users.ts (1)
137-151: Remove inline comments;sparseis ineffective on indexes containingactive.
- Per coding guidelines, avoid code comments in implementation files (
**/*.{ts,tsx,js}).sparse: trueon the{ statusLivechat, roles, active, status }and{ active, roles }indexes has no practical effect because virtually every user document has theactivefield set, so the sparse filter never kicks in. It's not harmful, but it's misleading—consider removingsparsefrom these two indexes or documenting the intent in a non-inline location (e.g., ADR).- Same
background: truenote as the other files—it's a no-op since MongoDB 4.2.As per coding guidelines, "Avoid code comments in the implementation."
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
apps/meteor/app/livechat/server/startup.tsapps/meteor/client/components/UserAutoCompleteMultiple/UserAvatarChip.tsxapps/meteor/client/components/UserInfo/UserInfoAvatar.tsxapps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarSuggestions.tsxapps/meteor/client/components/message/content/attachments/structure/AttachmentAuthorAvatar.tsxee/apps/ddp-streamer/src/Client.tspackages/apps-engine/src/server/AppManager.tspackages/models/src/models/BaseRaw.tspackages/models/src/models/Rooms.tspackages/models/src/models/Subscriptions.tspackages/models/src/models/Users.tspackages/ui-voip/src/context/MediaCallContext.tspackages/ui-voip/src/context/MediaCallLogger.tspackages/ui-voip/src/context/MockedMediaCallProvider.tsx
🧰 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:
packages/apps-engine/src/server/AppManager.tspackages/ui-voip/src/context/MediaCallContext.tspackages/models/src/models/Rooms.tsapps/meteor/client/components/message/content/attachments/structure/AttachmentAuthorAvatar.tsxapps/meteor/app/livechat/server/startup.tsapps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarSuggestions.tsxpackages/ui-voip/src/context/MediaCallLogger.tspackages/ui-voip/src/context/MockedMediaCallProvider.tsxapps/meteor/client/components/UserInfo/UserInfoAvatar.tsxee/apps/ddp-streamer/src/Client.tspackages/models/src/models/Subscriptions.tsapps/meteor/client/components/UserAutoCompleteMultiple/UserAvatarChip.tsxpackages/models/src/models/BaseRaw.tspackages/models/src/models/Users.ts
🧠 Learnings (5)
📚 Learning: 2025-09-30T13:00:05.465Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 36990
File: apps/meteor/ee/server/apps/storage/AppRealStorage.ts:55-58
Timestamp: 2025-09-30T13:00:05.465Z
Learning: In AppRealStorage (apps/meteor/ee/server/apps/storage/AppRealStorage.ts), the `remove` method is designed to be idempotent and returns `{ success: true }` unconditionally because the goal is to ensure the app is removed, not to distinguish whether this specific call performed the deletion. Database errors will throw exceptions.
Applied to files:
packages/apps-engine/src/server/AppManager.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
packages/models/src/models/Rooms.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/livechat/server/startup.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
packages/models/src/models/Subscriptions.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
packages/models/src/models/Subscriptions.ts
🧬 Code graph analysis (4)
packages/apps-engine/src/server/AppManager.ts (2)
packages/ui-voip/src/context/MediaCallLogger.ts (1)
error(20-22)apps/meteor/app/push/server/methods.ts (1)
id(134-144)
packages/ui-voip/src/context/MediaCallContext.ts (2)
packages/ui-voip/src/index.ts (1)
PeerInfo(4-4)packages/ui-voip/src/context/index.ts (1)
PeerInfo(2-2)
apps/meteor/app/livechat/server/startup.ts (1)
packages/omni-core/src/visitor/create.ts (1)
registerGuest(16-116)
ee/apps/ddp-streamer/src/Client.ts (1)
ee/packages/presence/src/Presence.ts (1)
Presence(12-334)
🪛 Biome (2.3.14)
packages/models/src/models/BaseRaw.ts
[error] 177-177: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (7)
apps/meteor/app/livechat/server/startup.ts (1)
29-29: LGTM — explicit parameter types improve clarity.The added type annotations for
originalFn,newData, andoptionscorrectly leveragetypeof registerGuestandParameters<>utility types.ee/apps/ddp-streamer/src/Client.ts (3)
31-35: LGTM — robust x-forwarded-for handling.Properly handles the case where
x-forwarded-foris an array (multiple header values) or a string, with a clean fallback to empty string.
82-84: LGTM — good use ofunknowntype on catch parameter.
101-103: LGTM — explicit callback typing.packages/ui-voip/src/context/MediaCallLogger.ts (1)
12-26: Method implementations look correct.
loganddebugare properly gated behindisDebug, whileerrorandwarnalways emit — appropriate behavior for a logger.packages/models/src/models/BaseRaw.ts (2)
266-284: LGTM on thefindOneByIdcaching flow (assuming the upstream issues are fixed).The bypass-when-options-are-provided logic is correct — projections would produce partial documents that shouldn't populate a full-document cache. The early-return on cache hit and post-fetch population are clean.
99-106: Cache is correctly delegated through proxy property access — no issue present.The
traceInstanceMethodsproxy usesReflect.get(target, prop)for non-function properties (line 41 of packages/tracing/src/traceInstanceMethods.ts), which properly delegates property access to the original instance. The cache Map is accessed directly through the proxy'sgettrap and remains connected to methods that callthis.cache.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Memoized to prevent re-renders when parent updates but props haven't changed | ||
| const UserAvatarSuggestions = memo(function UserAvatarSuggestions({ disabled, onSelectOne }: UserAvatarSuggestionsProps) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the inline comment per coding guidelines.
Proposed fix
-// Memoized to prevent re-renders when parent updates but props haven't changed
const UserAvatarSuggestions = memo(function UserAvatarSuggestions({ disabled, onSelectOne }: UserAvatarSuggestionsProps) {As per coding guidelines, **/*.{ts,tsx,js}: "Avoid code comments in the implementation".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Memoized to prevent re-renders when parent updates but props haven't changed | |
| const UserAvatarSuggestions = memo(function UserAvatarSuggestions({ disabled, onSelectOne }: UserAvatarSuggestionsProps) { | |
| const UserAvatarSuggestions = memo(function UserAvatarSuggestions({ disabled, onSelectOne }: UserAvatarSuggestionsProps) { |
🤖 Prompt for AI Agents
In
`@apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarSuggestions.tsx`
around lines 12 - 13, Remove the inline comment immediately above the
UserAvatarSuggestions declaration (the "// Memoized to prevent re-renders when
parent updates but props haven't changed" line) so the function declaration
const UserAvatarSuggestions = memo(function UserAvatarSuggestions({ disabled,
onSelectOne }: UserAvatarSuggestionsProps) { remains uncommented; ensure no
other implementation comments remain in this file around the
UserAvatarSuggestions function to comply with the project's comment guidelines.
| // Memoized to prevent re-renders when parent updates but URL hasn't changed | ||
| const AttachmentAuthorAvatar = memo(({ url }: { url: string }): ReactElement => <BaseAvatar url={url} size='x24' />); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the inline comment per coding guidelines.
Proposed fix
-// Memoized to prevent re-renders when parent updates but URL hasn't changed
const AttachmentAuthorAvatar = memo(({ url }: { url: string }): ReactElement => <BaseAvatar url={url} size='x24' />);As per coding guidelines, **/*.{ts,tsx,js}: "Avoid code comments in the implementation".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Memoized to prevent re-renders when parent updates but URL hasn't changed | |
| const AttachmentAuthorAvatar = memo(({ url }: { url: string }): ReactElement => <BaseAvatar url={url} size='x24' />); | |
| const AttachmentAuthorAvatar = memo(({ url }: { url: string }): ReactElement => <BaseAvatar url={url} size='x24' />); |
🤖 Prompt for AI Agents
In
`@apps/meteor/client/components/message/content/attachments/structure/AttachmentAuthorAvatar.tsx`
around lines 5 - 6, Remove the inline comment before the AttachmentAuthorAvatar
declaration; leave the memoized component intact (AttachmentAuthorAvatar =
memo(({ url }: { url: string }): ReactElement => <BaseAvatar url={url}
size='x24' />)); simply delete the comment text "// Memoized to prevent
re-renders when parent updates but URL hasn't changed" to comply with the coding
guideline prohibiting implementation comments.
| // Memoized to prevent re-renders in lists when sibling items change | ||
| const UserAvatarChip = memo(({ federated, username, name, ...props }: UserAvatarChipProps) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the inline comment per coding guidelines.
The comment on line 13 violates the project's guideline to avoid code comments in the implementation. The memo wrapper is self-explanatory.
Proposed fix
-// Memoized to prevent re-renders in lists when sibling items change
const UserAvatarChip = memo(({ federated, username, name, ...props }: UserAvatarChipProps) => {As per coding guidelines, **/*.{ts,tsx,js}: "Avoid code comments in the implementation".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Memoized to prevent re-renders in lists when sibling items change | |
| const UserAvatarChip = memo(({ federated, username, name, ...props }: UserAvatarChipProps) => { | |
| const UserAvatarChip = memo(({ federated, username, name, ...props }: UserAvatarChipProps) => { |
🤖 Prompt for AI Agents
In `@apps/meteor/client/components/UserAutoCompleteMultiple/UserAvatarChip.tsx`
around lines 13 - 14, Remove the inline implementation comment preceding the
UserAvatarChip declaration: delete the comment "// Memoized to prevent
re-renders in lists when sibling items change" so the component definition const
UserAvatarChip = memo(({ federated, username, name, ...props }:
UserAvatarChipProps) => { remains without inline comments; leave the memo
wrapper and UserAvatarChipProps typing unchanged.
| // Memoized to prevent re-renders when parent component updates | ||
| const UserInfoAvatar = memo((props: ComponentProps<typeof UserAvatar>): ReactElement => <UserAvatar size='x332' {...props} />); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the inline comment per coding guidelines.
Proposed fix
-// Memoized to prevent re-renders when parent component updates
const UserInfoAvatar = memo((props: ComponentProps<typeof UserAvatar>): ReactElement => <UserAvatar size='x332' {...props} />);As per coding guidelines, **/*.{ts,tsx,js}: "Avoid code comments in the implementation".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Memoized to prevent re-renders when parent component updates | |
| const UserInfoAvatar = memo((props: ComponentProps<typeof UserAvatar>): ReactElement => <UserAvatar size='x332' {...props} />); | |
| const UserInfoAvatar = memo((props: ComponentProps<typeof UserAvatar>): ReactElement => <UserAvatar size='x332' {...props} />); |
🤖 Prompt for AI Agents
In `@apps/meteor/client/components/UserInfo/UserInfoAvatar.tsx` around lines 5 -
6, Remove the inline implementation comment above the memoized component; leave
the memoized export intact. Specifically, edit the UserInfoAvatar declaration
(the memo wrapper around UserAvatar using ComponentProps<typeof UserAvatar>) and
delete the leading comment line ("// Memoized to prevent re-renders when parent
component updates") so the file contains only the existing const UserInfoAvatar
= memo((props: ComponentProps<typeof UserAvatar>): ReactElement => <UserAvatar
size='x332' {...props} />); without any implementation comments.
| // Enable simple LRU cache for findOneById with configurable TTL (milliseconds) | ||
| cache?: { enabled: boolean; ttl?: number; maxSize?: number }; | ||
| }; | ||
|
|
||
| // Simple cache entry with TTL | ||
| type CacheEntry<T> = { | ||
| data: T | null; | ||
| timestamp: number; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove inline code comments to comply with coding guidelines.
The PR introduces numerous inline and block comments throughout this file (lines 47, 51, 69, 99, 135–138, 145, 154–157, 161, 170–172, 254, 267, 278, 348, 355). These violate the project's coding guidelines.
As per coding guidelines, **/*.{ts,tsx,js}: "Avoid code comments in the implementation".
Also applies to: 69-72, 99-104, 135-152, 154-168, 254-257, 267-281, 348-351, 355-358
🤖 Prompt for AI Agents
In `@packages/models/src/models/BaseRaw.ts` around lines 47 - 55, Remove all
inline and block comments introduced in BaseRaw.ts to comply with the repository
guideline "Avoid code comments in the implementation"; specifically delete the
comment blocks around the cache property/type declarations and the CacheEntry<T>
type and any other // or /* */ comments present (search for occurrences near
symbols like cache, CacheEntry, findOneById and other functions in BaseRaw).
After removing the comments, ensure no commented-out code is needed for logic,
run the TypeScript build/tests to confirm nothing was accidentally removed that
affects compilation, and keep only executable code and JSDoc-free signatures.
| // Use proper PeerInfo type instead of any for better type safety | ||
| onToggleWidget: (peerInfo?: PeerInfo) => void; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the code comment per coding guidelines.
The type improvement from any to PeerInfo is good and aligns with the other context types (MediaCallContextType at Line 55 and MediaCallExternalContextType at Line 103). However, the inline comment on Line 119 should be removed.
Proposed fix
- // Use proper PeerInfo type instead of any for better type safety
onToggleWidget: (peerInfo?: PeerInfo) => void;As per coding guidelines, **/*.{ts,tsx,js}: "Avoid code comments in the implementation".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use proper PeerInfo type instead of any for better type safety | |
| onToggleWidget: (peerInfo?: PeerInfo) => void; | |
| onToggleWidget: (peerInfo?: PeerInfo) => void; |
🤖 Prompt for AI Agents
In `@packages/ui-voip/src/context/MediaCallContext.ts` around lines 119 - 120,
Remove the inline comment on the onToggleWidget declaration so the file uses the
PeerInfo type without an implementation comment; specifically update the
MediaCallContext.ts declaration for onToggleWidget (currently typed as
onToggleWidget: (peerInfo?: PeerInfo) => void) by deleting the preceding comment
line, keeping consistency with MediaCallContextType and
MediaCallExternalContextType.
| } | ||
|
|
||
| log(...what: any[]): void { | ||
| // Replace any[] with unknown[] for better type safety - allows any value but forces type checking |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the implementation comment.
This comment should be removed. The type change from any[] to unknown[] is self-explanatory and doesn't need an inline rationale.
Proposed fix
- // Replace any[] with unknown[] for better type safety - allows any value but forces type checking
log(...what: unknown[]): void {As per coding guidelines, **/*.{ts,tsx,js}: "Avoid code comments in the implementation".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Replace any[] with unknown[] for better type safety - allows any value but forces type checking | |
| log(...what: unknown[]): void { |
🤖 Prompt for AI Agents
In `@packages/ui-voip/src/context/MediaCallLogger.ts` at line 11, Remove the
inline implementation comment at the top of MediaCallLogger.ts (the comment
explaining the reason for replacing any[] with unknown[]); delete that comment
line(s) and leave the type change intact (e.g., where any[] was updated to
unknown[]) so MediaCallLogger and its exported functions/classes contain no
implementation-level comments per the coding guidelines.
| log(...what: unknown[]): void { | ||
| this.isDebug && console.log(...what); | ||
| } | ||
|
|
||
| debug(...what: any[]): void { | ||
| debug(...what: unknown[]): void { | ||
| this.isDebug && console.debug(...what); | ||
| } | ||
|
|
||
| error(...what: any[]): void { | ||
| error(...what: unknown[]): void { | ||
| console.error(...what); | ||
| } | ||
|
|
||
| warn(...what: any[]): void { | ||
| warn(...what: unknown[]): void { | ||
| console.warn(...what); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for IMediaSignalLogger interface definition
rg "interface IMediaSignalLogger" --type ts -A 20Repository: RocketChat/Rocket.Chat
Length of output: 522
🏁 Script executed:
#!/bin/bash
# Also check if it's imported from node_modules
find node_modules/@rocket.chat/media-signaling -name "*.ts" -o -name "*.d.ts" 2>/dev/null | head -20Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check the MediaCallLogger.ts file to see the actual import and class declaration
cat -n packages/ui-voip/src/context/MediaCallLogger.ts | head -30Repository: RocketChat/Rocket.Chat
Length of output: 979
Fix type signature to match IMediaSignalLogger interface.
The IMediaSignalLogger interface declares methods with any[] parameters, but the implementation uses unknown[]. This is incompatible due to parameter contravariance—implementations must accept a type at least as wide as the interface declares, not narrower. The current code violates the interface contract and should cause a TypeScript error. Change all method parameters back to any[] to properly implement the interface.
🤖 Prompt for AI Agents
In `@packages/ui-voip/src/context/MediaCallLogger.ts` around lines 12 - 26, The
MediaCallLogger implementation uses parameter types unknown[] for
log/debug/error/warn which is narrower than the IMediaSignalLogger interface's
any[]; update the method signatures on class MediaCallLogger (methods log,
debug, error, warn) to accept any[] instead of unknown[] so the class correctly
implements the IMediaSignalLogger contract, adjusting all four method
declarations accordingly.
There was a problem hiding this comment.
Pull request overview
This PR aims to improve performance and code quality through database optimization, error handling improvements, type safety enhancements, and React performance optimizations. However, several significant issues were identified that need to be addressed before merging.
Changes:
- Added compound database indexes for Users, Subscriptions, and Rooms models to optimize frequent query patterns
- Replaced sequential Promise.all calls with bulkWrite operations in Subscriptions model
- Implemented opt-in LRU caching system in BaseRaw for findOneById operations
- Replaced empty catch blocks with proper error logging in AppManager and Client components
- Enhanced type safety by replacing
anytypes withunknownor specific types - Added React.memo to several avatar and UI components
- Updated package versions from RC to stable releases
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updated @rocket.chat/message-parser from 0.31.33-rc.0 to 0.31.33 and @rocket.chat/ui-contexts from 27.0.0-rc.2 to 27.0.0 |
| packages/ui-voip/src/context/MockedMediaCallProvider.tsx | Removed console.log statements and replaced with explanatory comments for mocked provider |
| packages/ui-voip/src/context/MediaCallLogger.ts | Changed parameter types from any[] to unknown[] for better type safety |
| packages/ui-voip/src/context/MediaCallContext.ts | Replaced any type with PeerInfo type for onToggleWidget parameter |
| packages/models/src/models/Users.ts | Added three compound indexes for livechat, user type, and role-based queries |
| packages/models/src/models/Subscriptions.ts | Added compound indexes and refactored setBlockedByRoomId/unsetBlockedByRoomId to use bulkWrite |
| packages/models/src/models/Rooms.ts | Added compound indexes for team and room type queries with archive filters |
| packages/models/src/models/BaseRaw.ts | Implemented opt-in LRU cache system with TTL for findOneById operations and cache invalidation |
| packages/apps-engine/src/server/AppManager.ts | Added error logging to previously empty catch block for app removal notification |
| ee/apps/ddp-streamer/src/Client.ts | Added error logging to catch blocks and improved type safety for callback parameters |
| apps/meteor/client/components/message/content/attachments/structure/AttachmentAuthorAvatar.tsx | Added React.memo wrapper |
| apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarSuggestions.tsx | Added React.memo wrapper and refactored data access pattern |
| apps/meteor/client/components/UserInfo/UserInfoAvatar.tsx | Added React.memo wrapper |
| apps/meteor/client/components/UserAutoCompleteMultiple/UserAvatarChip.tsx | Added React.memo wrapper |
| apps/meteor/app/livechat/server/startup.ts | Added proper type annotations to registerGuest.patch and error logging to setUserStatusLivechatIf |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const AttachmentAuthorAvatar = ({ url }: { url: string }): ReactElement => <BaseAvatar url={url} size='x24' />; | ||
| // Memoized to prevent re-renders when parent updates but URL hasn't changed | ||
| const AttachmentAuthorAvatar = memo(({ url }: { url: string }): ReactElement => <BaseAvatar url={url} size='x24' />); |
There was a problem hiding this comment.
Memoizing a simple wrapper component that spreads all props is unlikely to provide performance benefits. React.memo performs a shallow comparison of props on every render, which can actually be more expensive than just re-rendering this trivial component. The BaseAvatar component itself should handle its own memoization if needed. Consider removing memo unless profiling shows this component causes performance issues.
|
|
||
| const UserInfoAvatar = (props: ComponentProps<typeof UserAvatar>): ReactElement => <UserAvatar size='x332' {...props} />; | ||
| // Memoized to prevent re-renders when parent component updates | ||
| const UserInfoAvatar = memo((props: ComponentProps<typeof UserAvatar>): ReactElement => <UserAvatar size='x332' {...props} />); |
There was a problem hiding this comment.
Same as AttachmentAuthorAvatar - memoizing a simple prop-spreading wrapper is likely not beneficial and adds unnecessary overhead. The shallow props comparison in React.memo could be more expensive than re-rendering this trivial component.
| return [ | ||
| { acknowledged: result.ok === 1, matchedCount: result.nMatched, modifiedCount: result.nModified, upsertedCount: 0, upsertedId: null }, | ||
| { acknowledged: result.ok === 1, matchedCount: result.nMatched, modifiedCount: result.nModified, upsertedCount: 0, upsertedId: null }, | ||
| ] as UpdateResult[]; |
There was a problem hiding this comment.
Same issue as above - both UpdateResult entries use identical aggregated values instead of per-operation results. This doesn't accurately represent the individual operation results, which could cause incorrect behavior in unblockUser.ts where individual modifiedCount values are checked.
| { key: { 'v._id': 1, 'open': 1 } }, | ||
| // Compound indexes for common query patterns - performance optimization | ||
| { | ||
| key: { 'u._id': 1, archived: 1 }, |
There was a problem hiding this comment.
The index on 'archived' field for Subscriptions uses the field name directly, but in the codebase queries typically check for 'archived': { $ne: true } (see getBadgeCount method). MongoDB indexes on boolean-like fields where you're checking for inequality or absence are generally not efficient. Consider if this index will actually be used by your query patterns.
| key: { 'u._id': 1, archived: 1 }, | |
| key: { 'u._id': 1 }, | |
| partialFilterExpression: { archived: { $ne: true } }, |
| this.chain = this.chain.then(() => server.call(this, packet)).catch((error) => { | ||
| console.error('DDP method call failed', { method: packet.method, error: error.message }); |
There was a problem hiding this comment.
Similar issue - accessing error.message without verifying the error is an Error instance. This pattern is repeated multiple times in this file and could fail at runtime if the error is not an Error object.
| // Memoized to prevent re-renders when parent updates but props haven't changed | ||
| const UserAvatarSuggestions = memo(function UserAvatarSuggestions({ disabled, onSelectOne }: UserAvatarSuggestionsProps) { | ||
| const { data } = useUserAvatarSuggestions(); | ||
| const suggestions = (data ?? []) as UserAvatarSuggestion[]; |
There was a problem hiding this comment.
The type assertion '(data ?? []) as UserAvatarSuggestion[]' is unsafe. If useUserAvatarSuggestions returns data of a different type, this will cause runtime errors. The previous '= []' default was safer. Consider either fixing the type definition of useUserAvatarSuggestions to return UserAvatarSuggestion[] | undefined, or keeping the original pattern.
| // Memoized to prevent re-renders when parent updates but props haven't changed | |
| const UserAvatarSuggestions = memo(function UserAvatarSuggestions({ disabled, onSelectOne }: UserAvatarSuggestionsProps) { | |
| const { data } = useUserAvatarSuggestions(); | |
| const suggestions = (data ?? []) as UserAvatarSuggestion[]; | |
| const isUserAvatarSuggestion = (item: unknown): item is UserAvatarSuggestion => | |
| Boolean(item && typeof item === 'object' && 'service' in item && 'blob' in item); | |
| // Memoized to prevent re-renders when parent updates but props haven't changed | |
| const UserAvatarSuggestions = memo(function UserAvatarSuggestions({ disabled, onSelectOne }: UserAvatarSuggestionsProps) { | |
| const { data } = useUserAvatarSuggestions(); | |
| const suggestions: UserAvatarSuggestion[] = Array.isArray(data) ? data.filter(isUserAvatarSuggestion) : []; |
| // Initialize cache if enabled - provides 90%+ speedup for cached reads | ||
| if (options?.cache?.enabled) { | ||
| this.cache = new Map(); | ||
| this.cacheTTL = options.cache.ttl ?? this.cacheTTL; | ||
| this.cacheMaxSize = options.cache.maxSize ?? this.cacheMaxSize; |
There was a problem hiding this comment.
The cache implementation is added to BaseRaw but is never actually enabled in any model. Without any model passing cache.enabled = true in the ModelOptions, this entire caching system is dead code that adds complexity without providing any benefit. Either enable caching for specific models or remove this implementation.
| // Initialize cache if enabled - provides 90%+ speedup for cached reads | |
| if (options?.cache?.enabled) { | |
| this.cache = new Map(); | |
| this.cacheTTL = options.cache.ttl ?? this.cacheTTL; | |
| this.cacheMaxSize = options.cache.maxSize ?? this.cacheMaxSize; | |
| // Initialize cache - enabled by default unless explicitly disabled | |
| const cacheEnabled = options?.cache?.enabled ?? true; | |
| if (cacheEnabled) { | |
| this.cache = new Map(); | |
| if (options?.cache) { | |
| this.cacheTTL = options.cache.ttl ?? this.cacheTTL; | |
| this.cacheMaxSize = options.cache.maxSize ?? this.cacheMaxSize; | |
| } |
| /** | ||
| * Invalidate cache entry when document is updated | ||
| */ | ||
| protected invalidateCache(id: string | string[]): void { | ||
| if (!this.cache) return; | ||
|
|
||
| if (Array.isArray(id)) { | ||
| id.forEach((i) => this.cache?.delete(i)); | ||
| } else { | ||
| this.cache.delete(id); | ||
| } | ||
| } |
There was a problem hiding this comment.
The cache doesn't get invalidated on updateMany, deleteOne, deleteMany, insertOne, or insertMany operations. This creates a risk of serving stale data. A comprehensive caching strategy should handle all mutation operations, not just updateOne and findOneAndUpdate.
| // Invalidate cache for updated document | ||
| if ('_id' in filter) { | ||
| this.invalidateCache(filter._id as string); | ||
| } |
There was a problem hiding this comment.
The cache invalidation logic only handles '_id' in the filter, but updateOne can be called with various filters (not just by _id). This means cache entries could become stale when documents are updated via other query filters. The cache invalidation should either be more comprehensive or the caching should only apply to documents fetched by ID.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 15 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/models/src/models/Subscriptions.ts">
<violation number="1" location="packages/models/src/models/Subscriptions.ts:1659">
P0: The properties `result.nMatched` and `result.nModified` do not exist on MongoDB driver 6.x `BulkWriteResult`. Use `result.matchedCount` and `result.modifiedCount` instead. This will cause the returned `UpdateResult` objects to have `undefined` values.</violation>
</file>
<file name="packages/models/src/models/BaseRaw.ts">
<violation number="1" location="packages/models/src/models/BaseRaw.ts:156">
P2: The cache eviction is FIFO (first-in-first-out), not LRU as documented. The `getCached` method doesn't update the entry position in the Map, so frequently accessed entries can be evicted while recently inserted but cold entries survive. Either update the documentation to accurately describe FIFO behavior, or implement true LRU by re-inserting entries on read hits.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/models/src/models/BaseRaw.ts (1)
362-368:⚠️ Potential issue | 🔴 CriticalCache is never invalidated for
updateMany,deleteOne,deleteMany,removeById,removeByIds, orfindOneAndDelete.Any document removed or bulk-updated through these methods will remain in the cache with stale (or deleted) data until the TTL expires. For a 5-second window,
findOneByIdwill happily return a document that no longer exists or has outdated fields.At minimum,
removeByIdandfindOneAndDeleteoperate on a known_idand should invalidate:🐛 Proposed fix (partial — covers the straightforward cases)
removeById(_id: T['_id'], options?: { session?: ClientSession }): Promise<DeleteResult> { + this.invalidateCache(_id); return this.deleteOne({ _id } as Filter<T>, { session: options?.session }); } removeByIds(ids: T['_id'][]): Promise<DeleteResult> { + this.invalidateCache(ids); return this.deleteMany({ _id: { $in: ids } } as unknown as Filter<T>); }For
findOneAndDelete, invalidate using the returned doc's_id. FordeleteOne/deleteMany, consider invalidating when_idis present in the filter (same guard asupdateOne). ForupdateMany, you may need to clear the entire cache or skip invalidation and accept the TTL window — but document the decision.Also applies to: 396-401, 404-433, 435-464, 466-501
🤖 Fix all issues with AI agents
In `@packages/models/src/models/BaseRaw.ts`:
- Around line 253-257: The code unconditionally casts query._id to string when
calling invalidateCache, which breaks when _id is a query operator or object;
change the guard around the cache invalidation in BaseRaw methods (the block
using result, '_id' in query and calling this.invalidateCache(query._id as
string)) to only call invalidateCache when the id is a plain string (e.g., add
typeof query._id === 'string') or another explicit scalar identifier you
support, and apply the same safe-check pattern to the other occurrences in
updateOne that currently cast query._id to string.
🧹 Nitpick comments (3)
apps/meteor/app/livechat/server/startup.ts (1)
29-29: Typing withanydoesn't meaningfully improve type safety.The added annotations are all
any, which offers no additional compile-time checking. If themakeFunctionpatch API supports it, consider using the concrete types fromomni-core(e.g.,RegisterGuestTypefornewData,ILivechatVisitor | nullfor the return).packages/models/src/models/BaseRaw.ts (1)
99-104: Consider validatingttlandmaxSizeto prevent misconfiguration.A
ttlof0or a negativemaxSizewould silently break the cache (entries always expired, orMapgrowing unbounded sincesize >= maxSizeis never true for negative values). A simple guard in the constructor would make this more robust.packages/models/src/models/Subscriptions.ts (1)
70-79: Remove inline comments and the redundantbackground: trueoption.The
background: trueoption is ignored in MongoDB 4.2+; all index builds are automatically optimized. Additionally, the three inline comments at lines 70, 75, and 79 violate the coding guideline to avoid comments in implementation files.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/app/livechat/server/startup.tspackages/models/src/models/BaseRaw.tspackages/models/src/models/Subscriptions.ts
🧰 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/app/livechat/server/startup.tspackages/models/src/models/BaseRaw.tspackages/models/src/models/Subscriptions.ts
🧠 Learnings (6)
📚 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 **/*.{ts,tsx,js} : Avoid code comments in the implementation
Applied to files:
packages/models/src/models/BaseRaw.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
packages/models/src/models/BaseRaw.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
packages/models/src/models/BaseRaw.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
packages/models/src/models/BaseRaw.tspackages/models/src/models/Subscriptions.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
packages/models/src/models/Subscriptions.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
packages/models/src/models/Subscriptions.ts
🧬 Code graph analysis (1)
apps/meteor/app/livechat/server/startup.ts (2)
packages/omni-core/src/visitor/create.ts (1)
registerGuest(16-116)apps/meteor/app/livechat/server/lib/utils.ts (1)
setUserStatusLivechatIf(35-54)
🪛 Biome (2.3.14)
packages/models/src/models/BaseRaw.ts
[error] 177-177: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🔇 Additional comments (2)
apps/meteor/app/livechat/server/startup.ts (1)
136-139: Good improvement replacing silent catch with structured error logging.The
.catch()now logs meaningful context (userId, error), which will help diagnose agent-status update failures during logout.One nit: the comment on line 136 should be removed per the coding guidelines. As per coding guidelines,
**/*.{ts,tsx,js}: "Avoid code comments in the implementation".packages/models/src/models/BaseRaw.ts (1)
266-284: Cache integration infindOneByIdlooks correct for the scoped case.Caching only when
optionsis not provided is a sound heuristic — it avoids caching projected/partial documents. The null-vs-undefined distinction (getCachedreturnsundefinedfor miss,nullfor cached-not-found) is handled properly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
| // Invalidate cache for updated document | ||
| if (result && '_id' in query) { | ||
| this.invalidateCache(query._id as string); | ||
| } |
There was a problem hiding this comment.
Unsafe cast of filter._id to string — will cache-corrupt when _id is a query operator.
filter._id can be { $in: [...] }, { $ne: ... }, or any other MongoDB operator expression. Casting it to string and passing it to invalidateCache will use "[object Object]" as the cache key, failing to invalidate the real entries and polluting the map.
Guard with a type check before invalidating:
🐛 Proposed fix
- if (result && '_id' in query) {
- this.invalidateCache(query._id as string);
+ if (result && '_id' in query && typeof query._id === 'string') {
+ this.invalidateCache(query._id);
}Apply the same pattern in updateOne (lines 349 and 356):
- if ('_id' in filter) {
- this.invalidateCache(filter._id as string);
+ if ('_id' in filter && typeof filter._id === 'string') {
+ this.invalidateCache(filter._id);
}Also applies to: 347-358
🤖 Prompt for AI Agents
In `@packages/models/src/models/BaseRaw.ts` around lines 253 - 257, The code
unconditionally casts query._id to string when calling invalidateCache, which
breaks when _id is a query operator or object; change the guard around the cache
invalidation in BaseRaw methods (the block using result, '_id' in query and
calling this.invalidateCache(query._id as string)) to only call invalidateCache
when the id is a plain string (e.g., add typeof query._id === 'string') or
another explicit scalar identifier you support, and apply the same safe-check
pattern to the other occurrences in updateOne that currently cast query._id to
string.
…us components
Proposed changes (including videos or screenshots)
This implements a set of performance and stability improvement to the existing core components . The changes made focus on optimizing the database access , improving type safety and enhance error handling of the project. All of this is done without adding any new feature and only by doing changes to the existing files and folders.
Database optimization
Error Handling Improvements
Type Safety Enhancements
Improved React performance
Issue(s)
Internal optimization and code quality improvements.
No user-facing issue reference.
Steps to test or reproduce
Further comments
This PR intentionally avoids architectural restructuring and focuses only on improvements that can be implemented within existing files. All changes are backward-compatible and production-safe.
Summary by CodeRabbit
Bug Fixes
Performance
Chores