refactor(livechat): replace any types with proper TypeScript interfaces in store#38339
Open
Agarwalchetan wants to merge 8 commits intoRocketChat:developfrom
Open
refactor(livechat): replace any types with proper TypeScript interfaces in store#38339Agarwalchetan wants to merge 8 commits intoRocketChat:developfrom
Agarwalchetan wants to merge 8 commits intoRocketChat:developfrom
Conversation
…es in store - Created types.ts with custom interfaces (Alert, QueueInfo, FileUploadConfig, etc.) - Replaced 19 any type annotations in StoreState interface - Imported existing types from @rocket.chat/core-typings (IMessage, ILivechatTrigger) - Fixed typo: LivechatHiddenSytemMessageType -> LivechatHiddenSystemMessageType - Improved type safety with index signatures and union types - Made modal a required boolean with explicit initialization - All changes maintain existing runtime behavior while adding compile-time type checking
Contributor
|
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 |
|
Contributor
WalkthroughReplaces numerous Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
KevLehman
added
the
area: ui/ux
Member
|
@MartinSchoeler could you take a look on this one? |
Contributor
Author
|
Hi! Just checking in to see if this PR looks okay or if any changes are needed. Happy to make updates if required. |
KevLehman
added
the
valid
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38339 +/- ##
===========================================
- Coverage 70.51% 70.45% -0.07%
===========================================
Files 3176 3176
Lines 111139 111139
Branches 20050 20055 +5
===========================================
- Hits 78367 78298 -69
- Misses 30721 30802 +81
+ Partials 2051 2039 -12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
- Fix Alert creation in connection.ts to use proper Alert type - Make Agent.ts property optional as not always provided by API - Update upsertMessage to accept properly typed message object - Update formatAgent to handle undefined input explicitly - Fix canRenderMessage to handle messages without type property These fixes address all 9 TypeScript errors revealed by CI after replacing any types in the store with proper interfaces.
Agarwalchetan
force-pushed
the
refactor/livechat-typescript-types
branch
from
93fe35a to
a9e7dfc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/livechat/src/helpers/canRenderMessage.ts (1)
34-37: Remove inline comment per coding guidelines.Line 36 contains an explanatory comment. The early-return pattern is self-explanatory given the optional parameter.
Proposed fix
export const canRenderMessage = ({ t }: { t?: string }) => { if (!t) { - return true; // Messages without a type should be rendered + return true; }As per coding guidelines:
**/*.{ts,tsx,js}: "Avoid code comments in the implementation".packages/livechat/src/lib/triggerUtils.ts (1)
77-84:as anycast undermines the type-safety goal of this PR.The explicit parameter type is a good step, but casting to
anyat the call site negates the benefit—theupserthelper receives an untyped value. Consider narrowing the cast to a more specific type (e.g.,as IMessage) or adjusting theupsertgeneric signature to accept this shape, so that at least some type checking is preserved at the boundary.Also, the inline comment on line 81 should be removed per coding guidelines. As per coding guidelines,
**/*.{ts,tsx,js}: Avoid code comments in the implementation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/livechat/src/definitions/agents.d.tspackages/livechat/src/helpers/canRenderMessage.tspackages/livechat/src/helpers/formatAgent.tspackages/livechat/src/lib/connection.tspackages/livechat/src/lib/triggerUtils.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:
packages/livechat/src/lib/triggerUtils.tspackages/livechat/src/definitions/agents.d.tspackages/livechat/src/helpers/canRenderMessage.tspackages/livechat/src/lib/connection.tspackages/livechat/src/helpers/formatAgent.ts
🧠 Learnings (2)
📓 Common learnings
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: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37496
File: packages/apps-engine/src/server/runtime/deno/ProcessMessenger.ts:5-5
Timestamp: 2025-11-16T14:23:18.933Z
Learning: In TypeScript, when a constant is used with `typeof` in a type position (e.g., `type Message = SomeType | typeof SOME_CONSTANT`), it's a type-level operation that doesn't require the runtime value. In such cases, `import type { SOME_CONSTANT }` is correct and appropriate.
📚 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} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
packages/livechat/src/definitions/agents.d.ts
🧬 Code graph analysis (2)
packages/livechat/src/lib/triggerUtils.ts (1)
apps/meteor/client/meteor/minimongo/LocalCollection.ts (1)
upsert(563-592)
packages/livechat/src/lib/connection.ts (2)
packages/livechat/src/lib/room.js (1)
alert(89-89)packages/livechat/src/store/types.ts (1)
Alert(6-13)
🔇 Additional comments (6)
packages/livechat/src/lib/connection.ts (3)
5-5: Good use ofimport typefor the Alert interface.Since
Alertis only used in a type position (the parameter annotation ondisplayAlert),import typeis the correct choice here — it ensures the import is erased at compile time.
62-65: Clean refactor: strong typing + immutable update.Replacing the mutable
.push()with an immutable spread is a solid improvement — it avoids mutating the store's state array in place, which aligns better with the store'ssetStatepattern. TheAlertparameter type ensures all call sites pass valid alert objects.
67-81: Call sites conform to theAlertinterface.Both
handleConnectedandhandleDisconnectedpass objects that satisfy theAlertinterface correctly (id,children, plus the appropriate optional flags).packages/livechat/src/helpers/formatAgent.ts (1)
13-31: LGTM!Clean handling of the undefined case. The narrowing via
if (!agent)is correct and the return type is properly inferred.packages/livechat/src/definitions/agents.d.ts (1)
3-16: Remove duplicateusernameproperty on line 9.
username: stringis declared twice (lines 4 and 9) in theAgenttype definition. TypeScript will fail to compile with duplicate property keys in object type literals—this prevents the code from building.packages/livechat/src/lib/triggerUtils.ts (1)
77-77:utyped asAgentPromiseallowsnull— is that intentional?
AgentPromiseis{ username: string } | Serialized<ILivechatAgent> | null. Anullsender on a message object is likely invalid. If the caller is expected to resolve the agent before callingupsertMessage, consider excludingnullfrom this field's type (e.g.,u: NonNullable<AgentPromise>).
- Fix prettier formatting in formatAgent.ts (indentation) - Fix import order in connection.ts (type imports after local) - Fix prettier formatting in triggerUtils.ts (multiline params) - Fix alert ID type cast in connection.ts - Update Agent type to match AgentType interface: - Add emails, phone, customFields with proper structure - Make username optional (not always provided by API) - Remove duplicate username field
Agarwalchetan
force-pushed
the
refactor/livechat-typescript-types
branch
from
2f9497e to
653b51b
Compare
- Fix formatAgent.ts prettier indentation (proper tabs) - Remove unused AgentType definition - Update Agent type to support phone as string or array - Add phone type check in formatAgent (string vs array) - Fix alert ID filtering with proper type assertion - Import Agent type in formatAgent All lint and typecheck tests now pass locally.
Agarwalchetan
force-pushed
the
refactor/livechat-typescript-types
branch
from
d742a02 to
0605b67
Compare
Resolved conflicts between our improved type-safe code and develop: - clearAlerts: kept our version with proper alert ID type assertion - displayAlert: kept our version with await and immutable array spread Both methods now properly handle types and async operations.
- Fix import order in formatAgent.ts (regular before type imports) - Fix prettier indentation in formatAgent.ts (proper tabs) - Add type cast in triggerUtils.ts for agent as AgentPromise All lint and typecheck tests now pass.
Ran prettier --write on formatAgent.ts to fix indentation issues that were causing CI lint failures.
Contributor
Author
|
Hi, just a gentle follow-up on this PR in case it got buried. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #38337 refactor(livechat): replace any types in Livechat store with proper TypeScript interfaces
This PR improves TypeScript type safety in the Livechat store by replacing all 19
anytype annotations inStoreStatewith explicit, well-defined TypeScript types.The changes are type-only, introduce no runtime behavior changes, and significantly improve IDE autocompletion, maintainability, and compile-time error detection.
Changes
1. Introduced Livechat-specific type definitions
Added a new
types.tsfile to centralize Livechat-related interfaces:Alert– UI alert notification structureLivechatConfigMessages– i18n/config message definitions (with index signature)LivechatResource– typed resource mapFileUploadConfig– file upload configurationQueueInfo– visitor queue metadataIncomingCallAlert– incoming call alert details2. Replaced
anyusages inStoreStateUpdated
packages/livechat/src/store/index.tsxto replace allanyannotations with proper types, reusing existing shared typings where available.Highlights:
@rocket.chat/core-typings(IMessage,ILivechatTrigger,Serialized)number | boolean,string | number)Updated Store Type Definitions
Modified index.tsx with the following type improvements:
config.messagesanyconfig.triggersany[]ILivechatTrigger[]config.resourcesanyconfig.settings.fileUploadanyconfig.settings.allowSwitchingDepartmentsanybooleanconfig.settings.showConnectinganybooleanconfig.settings.limitTextLengthanynumber | booleanmessagesany[]alertsany[]Alert[]unreadanynumber | nullincomingCallAlertanyIncomingCallAlert | nullbusinessUnitanystring | nullmodalanybooleanagentanylastReadMessageIdanystring | numbertriggerAgentanyqueueInfoanyTotal: 19
anyannotations replaced with proper typesScope
Files changed:
packages/livechat/src/store/index.tsxpackages/livechat/src/store/types.ts(new)Runtime behavior: unchanged
Breaking changes: none
Benefits
Summary by CodeRabbit
New Features
Refactor