fix: typescript migration helpers 6781 #6796
Conversation
- Removed @ts-nocheck directive - Added proper TypeScript type annotations to all 8 exported functions - Added JSDoc comments for better documentation - Imported necessary types from definitions - Functions now have complete type safety: * isGroupChat: room parameter with IRoom | ISubscription types * getRoomAvatar: returns string | undefined * getUidDirectMessage: handles null checks with proper return types * getRoomTitle: returns string | undefined * getSenderName: uses IUserMessage type * canAutoTranslate: returns boolean * isRead: uses ISubscription type * hasRole: accepts string parameter * hasPermission: properly typed async function with permissions array Fixes RocketChat#6781
- Updated helper function signatures to accept undefined inputs - Created TRoomLike type for flexible room-like object handling - Made return types consistent (string instead of string | undefined) - Added null checks in calling code (RoomActionsView, RoomInfoView, RoomView) - Fixed type compatibility between RoomType and SubscriptionType All TypeScript compilation errors are now resolved while maintaining type safety.
|
Warning Rate limit exceeded@DSingh0304 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughAdds TypeScript typings and null-safety to helpers in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
DSingh0304
changed the title
![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
🧹 Nitpick comments (2)
app/lib/methods/helpers/helpers.ts (2)
31-88:getUidDirectMessageandgetRoomTitleimprovements; consider guarding missing login userThe new
getUidDirectMessagebehavior (explicitstring | undefined,itsMesupport, group-chat early return) andgetRoomTitle’s federated/DM-aware logic nicely tighten nullability and make call sites safer.One potential hardening: destructuring from
reduxStore.getState().login.userwill throw iflogin.useris ever unset (e.g., early in app start). You could defensively guard that to fail closed instead of crashing:-export function getUidDirectMessage(room: (Partial<TRoomLike> & { itsMe?: boolean }) | null | undefined): string | undefined { - const { id: userId } = reduxStore.getState().login.user; +export function getUidDirectMessage(room: (Partial<TRoomLike> & { itsMe?: boolean }) | null | undefined): string | undefined { + const loginUser = reduxStore.getState().login.user; + const userId = loginUser?.id; + + if (!loginUser || !userId) { + return undefined; + }This keeps the current semantics for normal flows while making the helper more robust to state shape changes.
90-156: Permission/flags helpers are safer; minor comment/observability nitsThe revised
canAutoTranslate,isRead,hasRole, andhasPermissionimplementations all look correct and more null-safe:
canAutoTranslatenow cleanly bails out on settings/permission misconfiguration and logs unexpected errors.isRead’s use ofunread ?? 0andalert === trueavoids undefined pitfalls.hasPermissioncorrectly merges room and user roles, handles undefined permission entries, and returns an all‑false array on lookup or state errors.Two minor polish ideas:
- In
isRead, the comment “item is not archived and not opened” doesn’t matchitem.open === true; clarifying that comment to “item is not archived and is open” would avoid confusion.- In the
hasPermissioncatch block where the room isn’t found, using the sharedloghelper (or includingridin the message) instead of a bareconsole.logwould make it easier to trace permission issues in production logs.Both are optional and don’t affect correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/lib/methods/helpers/helpers.ts(4 hunks)app/views/RoomActionsView/index.tsx(1 hunks)app/views/RoomInfoView/index.tsx(1 hunks)app/views/RoomView/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/views/RoomActionsView/index.tsx (1)
app/lib/services/restApi.ts (1)
getUserInfo(1067-1070)
app/views/RoomView/index.tsx (1)
app/lib/services/restApi.ts (1)
getUserInfo(1067-1070)
app/views/RoomInfoView/index.tsx (1)
app/lib/services/restApi.ts (1)
getUserInfo(1067-1070)
app/lib/methods/helpers/helpers.ts (4)
app/definitions/IRoom.ts (1)
IRoom(18-65)app/definitions/ISubscription.ts (1)
ISubscription(40-119)app/sagas/deepLinking.js (4)
room(69-69)room(222-222)uid(226-226)item(71-76)app/definitions/IMessage.ts (1)
IUserMessage(23-27)
🔇 Additional comments (4)
app/views/RoomView/index.tsx (1)
616-637: Guard aroundroomUserIdcorrectly prevents invalidgetUserInfocallsWrapping the state update and
getUserInfocall inif (roomUserId)is a good safety net with the newgetUidDirectMessagebehavior, avoiding API calls and header updates when the DM target can’t be resolved, without changing the successful-path behavior.app/lib/methods/helpers/helpers.ts (1)
7-29:TRoomLike,isGroupChat, andgetRoomAvatartypings/logic look solidThe
TRoomLikehelper type cleanly abstracts the overlap betweenIRoomandISubscription, and the updatedisGroupChat/getRoomAvatarimplementations are null-safe and align with the “> 2 participants ⇒ group chat” rule, with sensible string fallbacks when metadata is missing.app/views/RoomInfoView/index.tsx (1)
164-183: GuardingroomUserIdinloadUsermatches new helper semanticsThe
if (roomUserId)guard beforegetUserInfocleanly aligns this code with the strictergetUidDirectMessagecontract and avoids sendingusers.inforequests with undefined IDs, while preserving the existing role-enrichment behavior for valid DMs.app/views/RoomActionsView/index.tsx (1)
465-482:updateRoomMembernow safely skips invalid DM member lookupsThe combination of
!isGroupChat(room)andif (roomUserId)before callinggetUserInfois a good tightening: it prevents requests with undefined IDs and intentionally avoids treating group DMs as single‑member conversations, while keeping the successful fetch path unchanged.
… props to be optional in UI components and helpers.
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)
app/lib/hooks/useUserData.ts (1)
24-24: Unsafe type assertion bypassesundefinedhandling.
getUidDirectMessagereturnsstring | undefined, but line 24 casts the result tostring. If the function returnsundefined, this could lead to runtime issues when setting state with an invaliduid.Apply this diff to handle the
undefinedcase safely:- const uid = (await getUidDirectMessage(room)) as string; + const uid = await getUidDirectMessage(room); + if (!uid) { + return; + }
🧹 Nitpick comments (1)
app/lib/methods/helpers/helpers.ts (1)
8-12: Consider refining the TRoomLike type definition.Using
Pick<IRoom | ISubscription, ...>with a union type may not work as expected. TypeScript will only pick properties that exist in both types. Consider using a union of Picks instead for more accurate typing:-type TRoomLike = Pick<IRoom | ISubscription, 'uids' | 'usernames' | 'prid' | 'fname' | 'name'> & { - t?: string; - rid?: string; - federated?: boolean; -}; +type TRoomLike = (Pick<IRoom, 'uids' | 'usernames' | 'prid' | 'fname' | 'name'> | Pick<ISubscription, 'uids' | 'usernames' | 'prid' | 'fname' | 'name'>) & { + t?: string; + rid?: string; + federated?: boolean; +};Alternatively, define it as a minimal interface:
type TRoomLike = { uids?: string[]; usernames?: string[]; prid?: string; fname?: string; name?: string; t?: string; rid?: string; federated?: boolean; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
app/containers/CallHeader.tsx(1 hunks)app/containers/InAppNotification/IncomingCallNotification/index.tsx(1 hunks)app/containers/InAppNotification/NotifierComponent.tsx(1 hunks)app/containers/RoomItem/interfaces.ts(4 hunks)app/lib/hooks/useUserData.ts(1 hunks)app/lib/methods/helpers/helpers.ts(4 hunks)app/views/RoomInfoView/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/RoomInfoView/index.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
app/lib/hooks/useUserData.ts (2)
app/sagas/encryption.js (2)
user(21-21)user(94-94)app/actions/login.ts (1)
setUser(91-96)
app/lib/methods/helpers/helpers.ts (3)
app/definitions/IRoom.ts (1)
IRoom(18-65)app/definitions/ISubscription.ts (1)
ISubscription(40-119)app/definitions/IMessage.ts (1)
IUserMessage(23-27)
🔇 Additional comments (14)
app/containers/InAppNotification/NotifierComponent.tsx (1)
26-26: LGTM! Optional avatar property is well-handled.The change to make
avataroptional aligns with the broader type-safety improvements. The fallback logic at line 82 (avatar = name) ensures a value is always provided to the Avatar component.app/lib/hooks/useUserData.ts (1)
10-16: LGTM! Explicit state type improves type safety.The explicit type annotation for the state correctly reflects that
avataris optional while other fields are required.app/containers/RoomItem/interfaces.ts (1)
44-44: LGTM! Consistent optional avatar handling across interfaces.All avatar-related properties and return types have been correctly updated to be optional, aligning with the
getRoomAvatarhelper that returnsstring | undefined. This ensures type consistency across the codebase.Also applies to: 97-97, 106-106, 152-152
app/lib/methods/helpers/helpers.ts (9)
17-19: LGTM! Well-typed with proper null-safety.The function correctly handles
undefinedinput and uses optional chaining to safely access properties. The JSDoc provides clear documentation.
24-29: LGTM! Properly returnsundefinedfor missing avatars.The function now correctly returns
string | undefinedand uses optional chaining throughout. This addresses the previous feedback about not returning empty strings.
34-58: LGTM! Improved type safety withundefinedreturns.The function now consistently returns
undefinedinstead ofnulland handles edge cases properly. The explicit|| undefinedat line 57 ensures the return type contract is maintained.
63-80: LGTM! Well-guarded string returns.The function uses optional chaining and provides empty string fallbacks appropriately. The federated room handling and special character logic are properly typed.
85-88: LGTM! Clean implementation with proper fallback.The function correctly uses the
IUserMessagetype and provides an empty string fallback for missing name/username.
93-106: LGTM! Robust error handling and boolean return.The function properly handles the feature flag check, permission verification, and includes try-catch with a safe
falsefallback.
111-115: LGTM! Null-safe with nullish coalescing.The function correctly uses nullish coalescing (
?? 0) to handle undefinedunreadvalues and properly types the parameter asPartial<ISubscription>.
120-124: LGTM! Simple and correctly typed.The function accepts a string parameter and performs the role check safely with array fallback.
132-157: LGTM! Comprehensive permission checking with proper error handling.The function now has explicit typing for the parameter and return type. The logic properly merges room and user roles, and the error handling returns safe defaults. Once the type assertion at line 141 is fixed, this will be fully type-safe.
app/containers/CallHeader.tsx (1)
22-22: LGTM! Consistent with avatar optionality pattern.Making the
avatarproperty optional aligns with the broader changes across the codebase. TheAvatarContainercomponent at line 74 receives the optional avatar via thetextprop, which should handleundefinedgracefully.app/containers/InAppNotification/IncomingCallNotification/index.tsx (1)
28-28: LGTM! Avatar optionality properly threaded through component hierarchy.Both the notification payload and the
IncomingCallHeaderprops correctly defineavataras optional. This allows the component to gracefully handle cases where avatar data is unavailable, and the value flows correctly to theCallHeadercomponent at line 79.Also applies to: 36-36
…mulator/app commands, and refine room role access.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/lib/methods/helpers/helpers.ts (5)
21-29: Group avatar identifier logic generally looks good; optional refinementReturning
undefinedinstead of a fallback empty string when there’s no avatar id is a good alignment with the previous review feedback. One small thing to double‑check: the extraroom?.uids && room?.usernamesguard means that group rooms detected viauidsbut missingusernames(or vice versa) will skip the group‑avatar branch and fall back tofname/name. If that’s not intentional, consider basing the key on whichever ofuidsorusernamesis available.
34-58: getUidDirectMessage flow is solid; consider guarding against missing login.userThe control flow covers
nullrooms,itsMe, legacyridDMs, and group DMs cleanly, and theundefinedreturns give clear “no DM target” semantics. The only potential fragility is destructuringconst { id: userId } = reduxStore.getState().login.user;which will throw iflogin.useris everundefinedwhen this helper runs. If that’s possible, you can make this safer with:- const { id: userId } = reduxStore.getState().login.user; + const userId = reduxStore.getState().login.user?.id;The rest of the logic already handles a falsy
userIdcorrectly.
63-80: getRoomTitle covers the main scenarios; minor simplification possibleThe ordering (federated → group usernames → special‑char rooms → DM/real‑name logic) reads well and guarantees a string result. Since
TRoomLikealready includes an optionalfederatedflag, you could simplify the first branch toif (room?.federated === true)and drop the'federated' in roomcheck without changing behavior.
108-115: isRead logic seems intentional; comment is slightly misleadingThe implementation treats items as unread only when they are not archived, are open, and have
unread > 0oralert === true, which is a reasonable definition. The comment on Line 112 (“not opened”) doesn’t matchitem.open === true, so it may be worth updating the comment (or condition, if the comment is the correct intent) to avoid confusion later.
132-158: hasPermission is now strongly typed and safer; unify logging styleUsing
db.get<TSubscriptionModel>('subscriptions')and accessingroom.rolesdirectly removes the previousas anyescape hatch and improves type safety. The mergedroomRoles/userRolesset plus the per‑permission mapping is clear. One minor nit: this function mixesconsole.log(room not found) withlog(e)in the catch; consider using the sameloghelper for both to keep logging consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/lib/methods/helpers/helpers.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/lib/methods/helpers/helpers.ts (3)
app/definitions/IRoom.ts (1)
IRoom(18-65)app/definitions/ISubscription.ts (2)
ISubscription(40-119)TSubscriptionModel(121-124)app/definitions/IMessage.ts (1)
IUserMessage(23-27)
🔇 Additional comments (4)
app/lib/methods/helpers/helpers.ts (4)
7-19: TRoomLike abstraction and isGroupChat typing look appropriateCombining
IRoom/ISubscriptionintoTRoomLikeand acceptingPartial<TRoomLike> | undefinedinisGroupChatgives you the flexibility you need across helpers while keeping the implementation null‑safe; the boolean logic for “more than 2 participants” is clear and consistent.
85-88: getSenderName correctly matches IUserMessage and settingsTyping
senderasIUserMessageand switching betweenname/usernamebased onUI_Use_Real_Name, with a safe''fallback, is exactly what you want here.
93-106: canAutoTranslate is defensive and respects role-based permissionsThe early exit on
AutoTranslate_Enabled, intersection ofpermissions['auto-translate']withuser.roles, and the try/catch that falls back tofalseall make this helper robust without surprising callers.
120-124: hasRole helper is simple and now properly typedExplicitly typing
role: stringand reading fromlogin.user.roleswith a safe fallback array keeps this helper straightforward and predictable.
…ate `getRoomTitle` to explicitly handle optional room titles.
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)
app/containers/RoomItem/index.tsx (1)
30-31: Move null-coercion to variable declaration for consistency.Line 31 constructs
testIDusing the rawnamevalue, which produces"rooms-list-view-item-undefined"whengetRoomTitlereturnsundefined. Meanwhile, line 61 coercesnameto an empty string. This inconsistency can break test selectors and accessibility tools.Apply this diff to unify the handling and improve the fallback:
-const name = getRoomTitle(item); +const name = getRoomTitle(item) || 'Unknown Room'; const testID = `rooms-list-view-item-${name}`;and remove the redundant coercion at the prop site:
- name={name || ''} + name={name}Optional: If you prefer an empty string for the name prop specifically, you can still unify at declaration and override at prop time, but a descriptive fallback like
'Unknown Room'improves UX when room titles are missing.Also applies to: 61-61
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
app/containers/RoomItem/index.tsx(1 hunks)app/containers/RoomItem/interfaces.ts(4 hunks)app/lib/methods/helpers/helpers.ts(4 hunks)app/views/AddExistingChannelView/index.tsx(2 hunks)app/views/CreateDiscussionView/SelectChannel.tsx(2 hunks)app/views/CreateDiscussionView/SelectUsers.tsx(2 hunks)app/views/ForwardMessageView/SelectPersonOrChannel.tsx(2 hunks)app/views/RoomInfoEditView/index.tsx(3 hunks)app/views/RoomView/index.tsx(3 hunks)app/views/ShareView/Header.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/views/AddExistingChannelView/index.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- app/containers/RoomItem/interfaces.ts
- app/views/RoomView/index.tsx
- app/lib/methods/helpers/helpers.ts
🧰 Additional context used
🧬 Code graph analysis (4)
app/views/CreateDiscussionView/SelectChannel.tsx (1)
app/lib/methods/helpers/helpers.ts (1)
getRoomTitle(63-80)
app/views/CreateDiscussionView/SelectUsers.tsx (1)
app/lib/methods/helpers/helpers.ts (1)
getRoomTitle(63-80)
app/containers/RoomItem/index.tsx (1)
app/sagas/deepLinking.js (1)
name(62-62)
app/views/ForwardMessageView/SelectPersonOrChannel.tsx (1)
app/lib/methods/helpers/helpers.ts (1)
getRoomTitle(63-80)
🪛 Biome (2.1.2)
app/views/RoomInfoEditView/index.tsx
[error] 453-453: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 454-454: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (5)
app/views/ShareView/Header.tsx (1)
96-96: LGTM! Proper null-safety guard.The fallback to an empty string ensures
setTitlealways receives a defined string value whengetRoomTitlereturnsundefined, preventing potential UI issues.app/views/CreateDiscussionView/SelectChannel.tsx (1)
32-32: LGTM! Consistent null-safety pattern.Both mappings now guarantee string values for the
textproperty by falling back to an empty string whengetRoomTitlereturnsundefined.Also applies to: 70-70
app/views/CreateDiscussionView/SelectUsers.tsx (1)
36-36: LGTM! Proper string coercion.The fallback ensures the
textproperty always receives a string value, aligning with the TypeScript migration's stricter return types.Also applies to: 69-69
app/views/ForwardMessageView/SelectPersonOrChannel.tsx (1)
32-32: LGTM! Consistent with project-wide pattern.Both room option mappings now provide string fallbacks, ensuring the UI components receive defined text values.
Also applies to: 65-65
app/views/RoomInfoEditView/index.tsx (1)
95-95: LGTM! Defensive string initialization.The fallback ensures the form name field always initializes with a string value, preventing
undefinedfrom being set.
…ssion handling robustness in UI components.
… improved type safety and permission handling.
2 participants
Proposed changes
This PR completes the TypeScript migration for
app/lib/methods/helpers/helpers.tsby removing the@ts-nocheckdirective and adding proper type annotations to all helper functions.Key changes:
@ts-nocheckdirective from helpers.tsTRoomLiketype alias to handle bothIRoomandISubscriptionobjects flexiblyundefinedinputs gracefully, ensuring type safetyRoomActionsView,RoomInfoView,RoomView) to prevent passingundefinedvalues togetUserInfoAll functions now have proper type annotations:
isGroupChat,getRoomAvatar,getUidDirectMessage,getRoomTitle,getSenderName,canAutoTranslate,isRead,hasRole,hasPermissionIssue(s)
Fixes #6781
How to test or reproduce
yarn tsc --noEmitto verify TypeScript compilation passes without errorsyarn lint app/lib/methods/helpers/helpers.tsto verify ESLint passesScreenshots
N/A - Type-only changes, no visual modifications
Types of changes
Checklist
Further comments
This is a type-safety improvement with no runtime behavior changes. The TypeScript migration ensures better code maintainability and catches potential type errors at compile time. All existing functionality is preserved while improving type safety across the codebase.
Verification completed:
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.