refactor: convert spotlight.js to TypeScript and fix object mutations#39234
refactor: convert spotlight.js to TypeScript and fix object mutations#39234SUMMERxKx wants to merge 1 commit intoRocketChat:developfrom
Conversation
- Renames spotlight.js to spotlight.ts - Adds explicit TypeScript interfaces for search parameters and results - Replaces mutable delete operator with object destructuring in fetchRooms - Replaces argument mutation in mapOutsiders and mapTeams with pure object return values - Preserves the existing search logic exactly as-is
|
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 |
|
WalkthroughThe file Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/server/lib/spotlight.ts (1)
203-210: Prefer explicit team payload mapping over spreading source objects.Spreading
tforwards internal fields (liketype) that are outsideSpotlightUserResult. Building the return shape explicitly keeps the API contract tighter and more stable.♻️ Suggested refactor
mapTeams(teams: { name: string; type: number }[]): SpotlightUserResult[] { - return teams.map((t) => ({ - ...t, - _id: t.name, + return teams.map(({ name }) => ({ + _id: name, + name, isTeam: true, - username: t.name, + username: name, status: 'online' as const, })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/spotlight.ts` around lines 203 - 210, The mapTeams function currently spreads the input team object t into the SpotlightUserResult, which leaks internal fields like type; update mapTeams to return an explicitly constructed SpotlightUserResult object (no spread) by listing only the required properties (_id, isTeam, username, status and any other fields defined on SpotlightUserResult) derived from t.name and literals, ensuring fields such as type are not forwarded; locate mapTeams in apps/meteor/server/lib/spotlight.ts and replace the spread-based mapping with the explicit property assignments matching the SpotlightUserResult shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/server/lib/spotlight.ts`:
- Around line 241-246: The function accepts userId?: string | null but passes it
directly to helpers expecting a non-null string; add a guard at the start of the
function to handle nullish userId (either early return [] or throw) before
invoking hasAllPermissionAsync, canAccessRoomAsync, _searchConnectedUsers, or
_searchTeams, and update callers accordingly so those helpers always receive a
string userId; specifically check userId for null/undefined and only call
hasAllPermissionAsync and canAccessRoomAsync when userId is a string, and skip
or adapt calls to _searchConnectedUsers and _searchTeams when userId is not
provided.
---
Nitpick comments:
In `@apps/meteor/server/lib/spotlight.ts`:
- Around line 203-210: The mapTeams function currently spreads the input team
object t into the SpotlightUserResult, which leaks internal fields like type;
update mapTeams to return an explicitly constructed SpotlightUserResult object
(no spread) by listing only the required properties (_id, isTeam, username,
status and any other fields defined on SpotlightUserResult) derived from t.name
and literals, ensuring fields such as type are not forwarded; locate mapTeams in
apps/meteor/server/lib/spotlight.ts and replace the spread-based mapping with
the explicit property assignments matching the SpotlightUserResult shape.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/server/lib/spotlight.ts
📜 Review details
🧰 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/server/lib/spotlight.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
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} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
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/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
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/`
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/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
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.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
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/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/server/lib/spotlight.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/server/lib/spotlight.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:
apps/meteor/server/lib/spotlight.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:
apps/meteor/server/lib/spotlight.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/server/lib/spotlight.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:
apps/meteor/server/lib/spotlight.ts
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/server/lib/spotlight.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/server/lib/spotlight.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: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/server/lib/spotlight.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/lib/spotlight.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/lib/spotlight.ts
🔇 Additional comments (2)
apps/meteor/server/lib/spotlight.ts (2)
14-47: Strong type contracts for spotlight payloads and search params.This is a solid TS migration step: the new result/parameter types make the data flow much clearer and safer.
49-55: Good immutable transformations replacing object mutation.Line 54 and Lines 115-116 correctly switch from in-place mutation patterns to pure object returns, reducing shared-reference side effects.
Also applies to: 115-117
| userId?: string | null; | ||
| rid?: string; | ||
| text: string; | ||
| usernames: string[]; | ||
| mentions?: boolean; | ||
| }): Promise<SpotlightUserResult[]> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine the spotlight.ts file
fd "spotlight.ts" --type f | head -5Repository: RocketChat/Rocket.Chat
Length of output: 146
🏁 Script executed:
#!/bin/bash
# Get the file size to understand how large it is
wc -l apps/meteor/server/lib/spotlight.tsRepository: RocketChat/Rocket.Chat
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Read the searchUsers method signature around lines 241-246
sed -n '230,260p' apps/meteor/server/lib/spotlight.tsRepository: RocketChat/Rocket.Chat
Length of output: 667
🏁 Script executed:
#!/bin/bash
# Read the context around the problematic lines: 339, 350, 354
sed -n '330,365p' apps/meteor/server/lib/spotlight.tsRepository: RocketChat/Rocket.Chat
Length of output: 822
🏁 Script executed:
#!/bin/bash
# Search for helper method definitions in spotlight.ts
rg -nP '(async\s+)?_searchConnectedUsers\s*\(|_searchTeams\s*\(' apps/meteor/server/lib/spotlight.ts -A 3Repository: RocketChat/Rocket.Chat
Length of output: 743
🏁 Script executed:
#!/antml:bash
# Get more context around helper method signatures
sed -n '1,240p' apps/meteor/server/lib/spotlight.ts | grep -n "_searchConnectedUsers\|_searchTeams\|private\|async" | tail -20Repository: RocketChat/Rocket.Chat
Length of output: 348
🏁 Script executed:
#!/bin/bash
# Search for all helper method definitions
rg -nP '(private|async).*(_searchConnectedUsers|_searchTeams)\s*\(' apps/meteor/server/lib/spotlight.ts -A 5Repository: RocketChat/Rocket.Chat
Length of output: 660
🏁 Script executed:
#!/bin/bash
# Check the full method to see if there's any early guard for userId
sed -n '234,360p' apps/meteor/server/lib/spotlight.ts | head -80Repository: RocketChat/Rocket.Chat
Length of output: 2017
🏁 Script executed:
#!/bin/bash
# Check if there's a return or guard before line 339
sed -n '234,345p' apps/meteor/server/lib/spotlight.tsRepository: RocketChat/Rocket.Chat
Length of output: 2951
🏁 Script executed:
#!/an/bin/bash
# Check if hasAllPermissionAsync requires userId: string
rg -nP 'hasAllPermissionAsync\s*\(' --type=ts -A 2 | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1989
🏁 Script executed:
#!/bin/bash
# Search for hasAllPermissionAsync signature
rg -nP 'export.*hasAllPermissionAsync|declare.*hasAllPermissionAsync' --type=ts -A 3Repository: RocketChat/Rocket.Chat
Length of output: 456
🏁 Script executed:
#!/bin/bash
# Look for hasAllPermissionAsync definition
fd . --type f -name "*.ts" | xargs grep -l "hasAllPermissionAsync" | head -5Repository: RocketChat/Rocket.Chat
Length of output: 296
🏁 Script executed:
#!/bin/bash
# Check canAccessRoomAsync signature
rg -nP 'export.*canAccessRoomAsync|declare.*canAccessRoomAsync' --type=ts -A 3Repository: RocketChat/Rocket.Chat
Length of output: 658
🏁 Script executed:
#!/bin/bash
# Also check the actual call to canAccessRoomAsync in spotlight.ts with more context
sed -n '270,280p' apps/meteor/server/lib/spotlight.tsRepository: RocketChat/Rocket.Chat
Length of output: 472
🏁 Script executed:
#!/bin/bash
# Check IUser['_id'] type definition
rg -nP "interface IUser|type IUser" --type=ts -A 5 | grep -A 5 "_id" | head -15Repository: RocketChat/Rocket.Chat
Length of output: 1179
🏁 Script executed:
#!/bin/bash
# Find the Authorization class canAccessRoom method
rg -nP 'canAccessRoom\s*\(' apps/meteor/app/authorization/server/ --type=ts -B 2 -A 8 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check if there's any null/undefined handling before the calls
sed -n '268,360p' apps/meteor/server/lib/spotlight.tsRepository: RocketChat/Rocket.Chat
Length of output: 2464
🏁 Script executed:
#!/bin/bash
# Get the full signature of hasAllPermissionAsync
sed -n '4,10p' apps/meteor/app/authorization/server/functions/hasPermission.tsRepository: RocketChat/Rocket.Chat
Length of output: 474
🏁 Script executed:
#!/bin/bash
# Check the full context around hasAllPermissionAsync and canAccessRoomAsync calls
sed -n '269,275p' apps/meteor/server/lib/spotlight.tsRepository: RocketChat/Rocket.Chat
Length of output: 329
Add userId guards before calling helpers that require string userId.
Line 241 allows userId to be undefined | null, but this nullable value is passed directly to hasAllPermissionAsync (line 271), canAccessRoomAsync (line 273), _searchConnectedUsers (lines 339, 350), and _searchTeams (line 354)—all expecting userId: string. This contract mismatch can leak nullish values into permission checks and lookups.
Additionally, canAccessRoomAsync at line 273 and hasAllPermissionAsync at line 271 also require guarding, not only the later helper calls.
🔧 Proposed fix
- const canListOutsiders = await hasAllPermissionAsync(userId, ['view-outside-room', 'view-d-room']);
- const canListInsiders = canListOutsiders || (rid && (await canAccessRoomAsync(room, { _id: userId })));
+ const canListOutsiders = userId ? (await hasAllPermissionAsync(userId, ['view-outside-room', 'view-d-room'])) : false;
+ const canListInsiders = canListOutsiders || (rid && userId && (await canAccessRoomAsync(room, { _id: userId })));
@@
- if (canListInsiders && rid) {
+ if (canListInsiders && rid && userId) {
// Search for insiders
if (await this._searchInsiderUsers(searchParams)) {
return users;
}
// Search for users that the requester has DMs with
if (await this._searchConnectedUsers(userId, searchParams, 'd')) {
return users;
}
}
@@
- } else if (await this._searchConnectedUsers(userId, searchParams, 'd')) {
+ } else if (userId && (await this._searchConnectedUsers(userId, searchParams, 'd'))) {
return users;
}
- if (await this._searchTeams(userId, searchParams)) {
+ if (userId && (await this._searchTeams(userId, searchParams))) {
return users;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/server/lib/spotlight.ts` around lines 241 - 246, The function
accepts userId?: string | null but passes it directly to helpers expecting a
non-null string; add a guard at the start of the function to handle nullish
userId (either early return [] or throw) before invoking hasAllPermissionAsync,
canAccessRoomAsync, _searchConnectedUsers, or _searchTeams, and update callers
accordingly so those helpers always receive a string userId; specifically check
userId for null/undefined and only call hasAllPermissionAsync and
canAccessRoomAsync when userId is a string, and skip or adapt calls to
_searchConnectedUsers and _searchTeams when userId is not provided.
Migrate
spotlight.jsto TypeScript and resolve direct object mutations within search methods to prevent potential side-effects on shared references.Proposed changes (including videos or screenshots)
This PR addresses significant technical debt in the core spotlight search module:
Moved
apps/meteor/server/lib/spotlight.jsto TypeScript:SpotlightUserResult,SpotlightRoomResult, andSpotlightSearchOptionsto document the exact projections being retrieved and utilized by the API caller (server/publications/spotlight.ts).Resolved Object Mutations:
The original JavaScript implementation altered method arguments and returned data structures directly, which can cause unexpected side-effects if references are shared in the Node event loop.
fetchRooms: Replaced the mutabledelete room.lastMessagewith object destructuring to return a new payload.mapOutsiders: Replacedu.outside = truewith a purereturn { ...u, outside: true }.mapTeams: Replaced direct property additions with a map returning a newly constructed object.Behavior Preservation:
options.limitdecrementing) and theusernamespayload injection inprocessLimitAndUsernameshave been logically preserved, as these patterns are tightly coupled to how the caller orchestrates search limit bounding.Issue(s)
Fixes #39233
Steps to test or reproduce
Further comments
This change is purely a language port and safety refactoring to align the codebase structure with Rocket.Chat's typing standard, reducing "plain JS" technical debt in core server components.
Summary by CodeRabbit
Release Notes