feat: Implement Model Context Protocol (MCP) Gateway and Tool Discovery #38096
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 |
|
WalkthroughIntroduces centralized UI error-tracking and fallback UIs, applies ErrorBoundary wrappers across app, and adds MCP integration with a gateway service and a new MCPToolList component. Error events are reported via a new errorTrackingService with scope and severity metadata. Changes
Sequence DiagramsequenceDiagram
participant Component
participant ErrorBoundary as Error Boundary
participant ETS as ErrorTrackingService
participant Console
Component->>ErrorBoundary: Runtime error thrown
ErrorBoundary->>ErrorBoundary: catchError(error, info)
ErrorBoundary->>ETS: reportError(error, { scope, severity, recoverable, componentPath })
ETS->>ETS: format metadata (severity mapping, [GLOBAL] label)
ETS->>Console: group/log message, stack, metadata
ErrorBoundary->>Component: render fallback UI (Global/Feature/Component)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 9 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/livechat/src/components/App/App.tsx">
<violation number="1" location="packages/livechat/src/components/App/App.tsx:6">
P1: The `react-error-boundary` package is imported but not declared in livechat's package.json dependencies. Additionally, this package uses Preact, not React, so `react-error-boundary` may not be compatible. Consider using a Preact-compatible error boundary implementation or configure preact/compat aliasing.</violation>
<violation number="2" location="packages/livechat/src/components/App/App.tsx:28">
P1: Cross-package imports via relative paths violate monorepo package boundaries. The livechat package should not reach into `ui-client/src/` directly. Either add `@rocket.chat/ui-client` as a dependency and import from its public exports, or create local error boundary components within the livechat package.</violation>
</file>
<file name="packages/ui-client/src/components/errors/ErrorFallbacks.tsx">
<violation number="1" location="packages/ui-client/src/components/errors/ErrorFallbacks.tsx:46">
P2: Hardcoded English string `'Component error'` breaks i18n consistency. Other components in this file properly use `useTranslation()`. Consider adding the translation hook to maintain localization support.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| import { Component } from 'preact'; | ||
| import Router, { route } from 'preact-router'; | ||
| import { withTranslation } from 'react-i18next'; | ||
| import { ErrorBoundary } from 'react-error-boundary'; |
There was a problem hiding this comment.
P1: The react-error-boundary package is imported but not declared in livechat's package.json dependencies. Additionally, this package uses Preact, not React, so react-error-boundary may not be compatible. Consider using a Preact-compatible error boundary implementation or configure preact/compat aliasing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/livechat/src/components/App/App.tsx, line 6:
<comment>The `react-error-boundary` package is imported but not declared in livechat's package.json dependencies. Additionally, this package uses Preact, not React, so `react-error-boundary` may not be compatible. Consider using a Preact-compatible error boundary implementation or configure preact/compat aliasing.</comment>
<file context>
@@ -3,6 +3,7 @@ import i18next from 'i18next';
import { Component } from 'preact';
import Router, { route } from 'preact-router';
import { withTranslation } from 'react-i18next';
+import { ErrorBoundary } from 'react-error-boundary';
import type { Department } from '../../definitions/departments';
</file context>
| alignItems='center' | ||
| justifyContent='center' | ||
| color='danger' | ||
| title='Component error' |
There was a problem hiding this comment.
P2: Hardcoded English string 'Component error' breaks i18n consistency. Other components in this file properly use useTranslation(). Consider adding the translation hook to maintain localization support.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ui-client/src/components/errors/ErrorFallbacks.tsx, line 46:
<comment>Hardcoded English string `'Component error'` breaks i18n consistency. Other components in this file properly use `useTranslation()`. Consider adding the translation hook to maintain localization support.</comment>
<file context>
@@ -0,0 +1,55 @@
+ alignItems='center'
+ justifyContent='center'
+ color='danger'
+ title='Component error'
+ borderWidth='default'
+ borderColor='danger'
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @apps/meteor/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsx:
- Line 5: The tools prop in MCPToolList is typed as any[] which removes type
safety; define a proper interface (e.g., MCPTool with name: string and
description: string) and change the prop signature to use that interface so
accesses like tool.name and tool.description are statically typed; update the
component signature (MCPToolList) to accept tools: MCPTool[] and import/export
the interface or declare it above the component as appropriate.
- Around line 19-27: Define a proper Tool interface and type the MCPToolList
component's props (replace any[] with Tool[]), then defensively validate/filter
tools before rendering: in MCPToolList filter out entries that are
null/undefined or where name is empty, map with a stable key (use a unique id
property if available or fallback to `${tool.name}-${index}`), and use fallbacks
for display values (e.g., tool.name ?? 'Unknown Tool', tool.description ?? '').
Update the mapping over tools (the tools.map loop that renders Option and uses
tool.name) to use these validated/fallback values to avoid relying on
potentially malformed data.
In @packages/core-services/src/mcp/MCPGatewayService.ts:
- Around line 9-43: The MCPGatewayService is missing cleanup/disconnect methods
and uses console logging; add two methods on the class—public async
disconnectServer(serverId: string): Promise<void> which looks up
this.clients.get(serverId), calls await client.disconnect?.(), deletes the
client from this.clients and reports failures via the project's
ErrorTrackingService or structured logger (not console.error), and public async
disconnectAll(): Promise<void> which iterates this.clients.keys() and awaits
Promise.allSettled(keys.map(id => this.disconnectServer(id))); also replace
console.log and console.error inside connectServer with the centralized
ErrorTrackingService/logger calls (include contextual messages and error
details) to align with production error tracking.
- Around line 33-40: getAllTools currently fails if any client.listTools()
throws; update it to catch errors per-client so one misbehaving MCP server
doesn't block aggregation: inside getAllTools iterate over
this.clients.values(), call await client.listTools() inside a try/catch, on
success push results into allTools, on error log the failure including a
client-identifying symbol (e.g., client.id, client.name or the client object)
and continue to the next client, then return the aggregated allTools as before.
- Line 10: The clients Map in MCPGatewayService is currently typed as
Map<string, any>; change its declaration to Map<string, MCPClient> to restore
type safety, update the initialization to new Map<string, MCPClient>(), and
import or reference the MCPClient type/interface where needed; then fix any call
sites that relied on implicit any by ensuring stored values are actual MCPClient
instances or cast/constructed appropriately (e.g., in methods that add or
retrieve clients, adjust signatures/returns to use MCPClient instead of any).
- Around line 15-28: In connectServer, prevent resource leaks and improve error
handling by first validating config.url (e.g., using the URL constructor) and
throwing immediately for malformed URLs; if this.clients has an existing client
for config.serverId, call its disconnect/close method (the existing MCPClient
instance) and remove it before creating a new MCPClient; attempt to connect the
new MCPClient and on any connection error ensure you clean up (disconnect/close
the new client if partially connected) and rethrow the error so callers can
handle failures instead of only logging; update references to connectServer,
MCPClient, MCPConfig, and this.clients accordingly.
In @packages/livechat/src/components/App/App.tsx:
- Around line 3-6: The file imports ErrorBoundary from 'react-error-boundary' in
App.tsx but the package is not listed in packages/livechat/package.json; either
add "react-error-boundary" to the livechat package dependencies and run the
package manager install, or replace the usage/import of ErrorBoundary in App.tsx
with a Preact-compatible alternative (e.g., implement a small Preact
ErrorBoundary component or use a preact-compatible library), updating the import
and any usage sites accordingly so the module resolution succeeds.
🧹 Nitpick comments (7)
packages/ui-client/src/components/errors/ErrorFallbacks.tsx (1)
39-54: Internationalization missing forComponentErrorFallbacktitle.The
title='Component error'attribute is hardcoded in English while other components uset()for translations. This inconsistency may cause issues in non-English locales.Additionally, the emoji
⚠️may not render consistently across all platforms/browsers. Consider usingStatesIconfor consistency with other fallbacks.♻️ Suggested improvement
-export const ComponentErrorFallback = () => ( +export const ComponentErrorFallback = () => { + const { t } = useTranslation(); + return ( <Box display='inline-flex' alignItems='center' justifyContent='center' color='danger' - title='Component error' + title={t('Component_error')} borderWidth='default' borderColor='danger' borderRadius='x4' p='x2' m='x2' > ⚠️ </Box> -); + ); +};packages/ui-client/src/services/__test__/ErrorTrackingService.test.ts (2)
5-6: Use proper vitest spy types instead ofany.The spy variables can use
ReturnType<typeof vi.spyOn>orMockInstancefrom vitest for better type safety.♻️ Suggested improvement
+import { describe, it, expect, vi, beforeEach, afterEach, type MockInstance } from 'vitest'; import { errorTrackingService, ErrorMetadata } from '../ErrorTrackingService'; describe('ErrorTrackingService', () => { - let consoleGroupSpy: any; - let consoleErrorSpy: any; + let consoleGroupSpy: MockInstance; + let consoleErrorSpy: MockInstance;
17-31: Consider assertingconsole.infofor metadata logging.The
reportErrormethod logs metadata viaconsole.info, but the test only verifiesconsole.groupandconsole.error. Adding an assertion forconsole.infowould improve coverage.♻️ Suggested improvement
it('should log errors with correct metadata to the console', () => { const testError = new Error('Test Crash'); const metadata: ErrorMetadata = { scope: 'feature', severity: 'high', recoverable: true, componentPath: 'WorkspacePage', }; + const consoleInfoSpy = vi.spyOn(console, 'info').mockImplementation(() => {}); errorTrackingService.reportError(testError, metadata); expect(consoleGroupSpy).toHaveBeenCalled(); expect(consoleErrorSpy).toHaveBeenCalledWith('Message:', 'Test Crash'); expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining('Stack Trace:'), expect.any(String)); + expect(consoleInfoSpy).toHaveBeenCalledWith('Metadata:', metadata); });packages/livechat/src/components/App/App.tsx (1)
28-29: Deep relative imports are fragile and error-prone.Importing from
../../../../ui-client/src/...creates tight coupling to directory structure. Consider using workspace package imports or aliased paths.♻️ Suggested improvement
If
@rocket.chat/ui-clientis set up as a workspace package, prefer:-import { GlobalErrorFallback } from '../../../../ui-client/src/components/errors/ErrorFallbacks'; -import { errorTrackingService } from '../../../../ui-client/src/services/ErrorTrackingService'; +import { GlobalErrorFallback } from '@rocket.chat/ui-client'; +import { errorTrackingService } from '@rocket.chat/ui-client';This requires exporting these from the package's public API.
packages/fuselage-ui-kit/src/surfaces/FuselageMessageSurfaceRenderer.tsx (1)
8-9: Deep relative imports should use package paths.Similar to the livechat package, these imports traverse multiple directories. Use workspace package imports for maintainability.
♻️ Suggested improvement
-import { ComponentErrorFallback } from '../components/errors/ErrorFallbacks'; -import { errorTrackingService } from '../services/ErrorTrackingService'; +import { ComponentErrorFallback, errorTrackingService } from '@rocket.chat/ui-client';packages/ui-client/src/services/ErrorTrackingService.ts (1)
8-23: Consider adding error deduplication for production use.The current implementation logs every error. In production, rapid error storms could flood logs. Consider tracking recent errors and throttling duplicates.
♻️ Optional enhancement for error deduplication
class ErrorTrackingService { private recentErrors = new Map<string, number>(); private readonly DEDUP_WINDOW_MS = 5000; public reportError(error: Error, metadata: ErrorMetadata): void { const errorKey = `${error.message}:${metadata.componentPath}`; const now = Date.now(); const lastSeen = this.recentErrors.get(errorKey); if (lastSeen && now - lastSeen < this.DEDUP_WINDOW_MS) { return; // Skip duplicate within window } this.recentErrors.set(errorKey, now); // ... existing logging logic } }apps/meteor/client/components/withErrorBoundary.tsx (1)
15-22: Severity mapping could be more granular.The current logic maps only
'global'to'critical'and everything else to'high'. Consider adding'medium'for'component'scope to better reflect the actual impact level, which would align with howFuselageMessageSurfaceRendereruses'medium'severity for component errors.♻️ Suggested improvement
onError={(error) => { errorTrackingService.reportError(error, { scope, - severity: scope === 'global' ? 'critical' : 'high', + severity: scope === 'global' ? 'critical' : scope === 'feature' ? 'high' : 'medium', recoverable: true, componentPath: Component.displayName || Component.name, }); }}
📜 Review details
Configuration used: Organization 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 (9)
apps/meteor/client/components/withErrorBoundary.tsxapps/meteor/client/views/admin/workspace/WorkspacePage.tsxapps/meteor/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsxpackages/core-services/src/mcp/MCPGatewayService.tspackages/fuselage-ui-kit/src/surfaces/FuselageMessageSurfaceRenderer.tsxpackages/livechat/src/components/App/App.tsxpackages/ui-client/src/components/errors/ErrorFallbacks.tsxpackages/ui-client/src/services/ErrorTrackingService.tspackages/ui-client/src/services/__test__/ErrorTrackingService.test.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/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsxpackages/ui-client/src/services/ErrorTrackingService.tspackages/core-services/src/mcp/MCPGatewayService.tspackages/ui-client/src/components/errors/ErrorFallbacks.tsxpackages/ui-client/src/services/__test__/ErrorTrackingService.test.tspackages/fuselage-ui-kit/src/surfaces/FuselageMessageSurfaceRenderer.tsxapps/meteor/client/views/admin/workspace/WorkspacePage.tsxpackages/livechat/src/components/App/App.tsxapps/meteor/client/components/withErrorBoundary.tsx
🧠 Learnings (11)
📚 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 apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
packages/ui-client/src/services/__test__/ErrorTrackingService.test.ts
📚 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 apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
packages/ui-client/src/services/__test__/ErrorTrackingService.test.ts
📚 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 apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/ui-client/src/services/__test__/ErrorTrackingService.test.ts
📚 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 apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
packages/ui-client/src/services/__test__/ErrorTrackingService.test.ts
📚 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 apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
packages/ui-client/src/services/__test__/ErrorTrackingService.test.ts
📚 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 apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
packages/ui-client/src/services/__test__/ErrorTrackingService.test.ts
📚 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 **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
packages/ui-client/src/services/__test__/ErrorTrackingService.test.ts
📚 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 apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
packages/ui-client/src/services/__test__/ErrorTrackingService.test.ts
📚 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 apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
packages/ui-client/src/services/__test__/ErrorTrackingService.test.ts
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
packages/fuselage-ui-kit/src/surfaces/FuselageMessageSurfaceRenderer.tsx
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/client/views/admin/workspace/WorkspacePage.tsx
🧬 Code graph analysis (5)
apps/meteor/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsx (2)
packages/ui-contexts/src/index.ts (1)
useTranslation(81-81)packages/ui-kit/src/index.ts (1)
Option(2-2)
packages/fuselage-ui-kit/src/surfaces/FuselageMessageSurfaceRenderer.tsx (4)
packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx (2)
renderTextObject(44-56)context(113-123)packages/ui-kit/src/index.ts (2)
VideoConferenceBlock(42-42)BlockContext(56-56)packages/ui-client/src/components/errors/ErrorFallbacks.tsx (1)
ComponentErrorFallback(40-55)packages/ui-client/src/services/ErrorTrackingService.ts (1)
errorTrackingService(25-25)
apps/meteor/client/views/admin/workspace/WorkspacePage.tsx (2)
apps/meteor/client/components/withErrorBoundary.tsx (1)
withErrorBoundary(7-31)packages/ui-client/src/components/errors/ErrorFallbacks.tsx (1)
FeatureErrorFallback(23-37)
packages/livechat/src/components/App/App.tsx (2)
packages/ui-client/src/components/errors/ErrorFallbacks.tsx (1)
GlobalErrorFallback(6-20)packages/ui-client/src/services/ErrorTrackingService.ts (1)
errorTrackingService(25-25)
apps/meteor/client/components/withErrorBoundary.tsx (3)
apps/meteor/app/cloud/server/functions/getCheckoutUrl.ts (1)
fallback(9-9)packages/ui-client/src/components/errors/ErrorFallbacks.tsx (1)
ComponentErrorFallback(40-55)packages/ui-client/src/services/ErrorTrackingService.ts (1)
errorTrackingService(25-25)
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (10)
packages/core-services/src/mcp/MCPGatewayService.ts (1)
1-1: Add @modelcontextprotocol/sdk to package.json dependencies.The @modelcontextprotocol/sdk is a real, well-documented npm package. The MCPClient API surface matches the usage here (constructor accepts config parameters,
connect()andlistTools()methods exist and are properly implemented). However, the dependency is missing from package.json, which will cause a runtime module-not-found error. The "Hypothetical SDK usage" comment should be removed once the dependency is properly declared.Likely an incorrect or invalid review comment.
apps/meteor/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsx (1)
3-3: Verify the import path for useTranslation.The component imports
useTranslationfrom'react-i18next', but the codebase may export it frompackages/ui-contexts/src/index.ts. Importing directly fromreact-i18nextmay bypass any project-specific wrappers or configuration. Confirm the correct import path by checking howuseTranslationis used in other components.Suggested fix
-import { useTranslation } from 'react-i18next'; +import { useTranslation } from '@rocket.chat/ui-contexts';packages/ui-client/src/components/errors/ErrorFallbacks.tsx (2)
5-20: Well-structured global error fallback.The component correctly provides a full-page fallback with appropriate styling, internationalization, and a clear recovery action via page reload.
22-37: LGTM!The
FeatureErrorFallbackcomponent is well-designed with proper prop typing forresetErrorBoundaryand consistent use of i18n.packages/livechat/src/components/App/App.tsx (1)
208-228: Error boundary integration looks correct.The
ErrorBoundaryproperly wraps the router with appropriate fallback and error reporting metadata. The scope is correctly set to'global'with critical severity for the livechat app root.packages/fuselage-ui-kit/src/surfaces/FuselageMessageSurfaceRenderer.tsx (1)
20-42: Error boundary implementation for video_conf block looks good.The error handling pattern is correct with appropriate scope (
'component'), medium severity, and recoverable flag. Thekeyplacement onErrorBoundaryis acceptable for React's reconciliation.One minor note: there's an extra blank line at line 23 before
<ErrorBoundary>that could be removed.apps/meteor/client/views/admin/workspace/WorkspacePage.tsx (2)
80-84: Good use ofwithErrorBoundaryHOC for feature-level error handling.The wrapper correctly applies the feature scope with
FeatureErrorFallbackand a reload-based reset strategy. UsingmemoinsidewithErrorBoundaryis a good pattern to avoid unnecessary re-renders.
9-10: Fix incorrect import paths on lines 9-10.Line 9 imports
FeatureErrorFallbackfrom a non-existent local path (../../../components/errors/ErrorFallbacks). The component is defined inpackages/ui-client/src/components/errors/ErrorFallbacks.tsxand not exported from the public API. Line 10 importswithErrorBoundaryfrom../../../lib/withErrorBoundary, but the actual file is atapps/meteor/client/components/withErrorBoundary.tsx—other files in the codebase correctly import from../../../components/withErrorBoundary.⛔ Skipped due to learnings
Learnt from: gabriellsh Repo: RocketChat/Rocket.Chat PR: 37834 File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22 Timestamp: 2025-12-16T17:29:45.163Z Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright testsLearnt 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 filespackages/ui-client/src/services/ErrorTrackingService.ts (1)
1-6: Well-defined error metadata interface.The
ErrorMetadatainterface provides a clear contract for error reporting with appropriate scope levels and severity classifications.apps/meteor/client/components/withErrorBoundary.tsx (1)
7-30: Well-structured error boundary HOC.The implementation provides a clean API for wrapping components with error handling, proper displayName assignment for React DevTools, and integration with the error tracking service. The generic typing preserves component prop types correctly.
apps/meteor/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsx
Outdated
Show resolved
Hide resolved
| class MCPGatewayService { | ||
| private clients: Map<string, any> = new Map(); | ||
|
|
||
| /** | ||
| * Connects to a new MCP server provided by the user/admin | ||
| */ | ||
| public async connectServer(config: MCPConfig): Promise<void> { | ||
| if (!config.enabled) return; | ||
|
|
||
| try { | ||
| // Initialize connection to the remote MCP server | ||
| const client = new MCPClient({ url: config.url }); | ||
| await client.connect(); | ||
|
|
||
| this.clients.set(config.serverId, client); | ||
| console.log(`[MCP] Connected to server: ${config.serverId}`); | ||
| } catch (error) { | ||
| console.error(`[MCP] Connection failed for ${config.serverId}:`, error); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Lists available tools from all connected MCP servers | ||
| */ | ||
| public async getAllTools() { | ||
| const allTools = []; | ||
| for (const client of this.clients.values()) { | ||
| const tools = await client.listTools(); | ||
| allTools.push(...tools); | ||
| } | ||
| return allTools; | ||
| } | ||
| } | ||
|
|
||
| export const mcpGatewayService = new MCPGatewayService(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add cleanup methods and replace console logging.
To align with the PR's error tracking infrastructure and production-grade requirements:
- Missing cleanup/disconnect: No method exists to disconnect individual servers or clean up all connections during shutdown, risking resource leaks in long-running processes.
- Console logging: Using
console.log/console.errorcontradicts the PR's structured error tracking service mentioned in commit messages and AI summary.
♻️ Proposed additions
Add cleanup methods:
/**
* Disconnects a specific MCP server
*/
public async disconnectServer(serverId: string): Promise<void> {
const client = this.clients.get(serverId);
if (client) {
try {
await client.disconnect?.();
this.clients.delete(serverId);
} catch (error) {
console.error(`[MCP] Failed to disconnect ${serverId}:`, error);
}
}
}
/**
* Disconnects all MCP servers (cleanup on shutdown)
*/
public async disconnectAll(): Promise<void> {
const disconnectPromises = Array.from(this.clients.keys()).map(
serverId => this.disconnectServer(serverId)
);
await Promise.allSettled(disconnectPromises);
}Replace console logging with your error tracking service (refer to the ErrorTrackingService mentioned in the PR context).
🤖 Prompt for AI Agents
In @packages/core-services/src/mcp/MCPGatewayService.ts around lines 9 - 43, The
MCPGatewayService is missing cleanup/disconnect methods and uses console
logging; add two methods on the class—public async disconnectServer(serverId:
string): Promise<void> which looks up this.clients.get(serverId), calls await
client.disconnect?.(), deletes the client from this.clients and reports failures
via the project's ErrorTrackingService or structured logger (not console.error),
and public async disconnectAll(): Promise<void> which iterates
this.clients.keys() and awaits Promise.allSettled(keys.map(id =>
this.disconnectServer(id))); also replace console.log and console.error inside
connectServer with the centralized ErrorTrackingService/logger calls (include
contextual messages and error details) to align with production error tracking.
| } | ||
|
|
||
| class MCPGatewayService { | ||
| private clients: Map<string, any> = new Map(); |
There was a problem hiding this comment.
Replace any with MCPClient for type safety.
Typing the Map value as any eliminates TypeScript's type checking, allowing potential runtime errors from incorrect method calls or type mismatches.
🔧 Proposed fix
- private clients: Map<string, any> = new Map();
+ private clients: Map<string, MCPClient> = new Map();📝 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.
| private clients: Map<string, any> = new Map(); | |
| private clients: Map<string, MCPClient> = new Map(); |
🤖 Prompt for AI Agents
In @packages/core-services/src/mcp/MCPGatewayService.ts at line 10, The clients
Map in MCPGatewayService is currently typed as Map<string, any>; change its
declaration to Map<string, MCPClient> to restore type safety, update the
initialization to new Map<string, MCPClient>(), and import or reference the
MCPClient type/interface where needed; then fix any call sites that relied on
implicit any by ensuring stored values are actual MCPClient instances or
cast/constructed appropriately (e.g., in methods that add or retrieve clients,
adjust signatures/returns to use MCPClient instead of any).
| public async connectServer(config: MCPConfig): Promise<void> { | ||
| if (!config.enabled) return; | ||
|
|
||
| try { | ||
| // Initialize connection to the remote MCP server | ||
| const client = new MCPClient({ url: config.url }); | ||
| await client.connect(); | ||
|
|
||
| this.clients.set(config.serverId, client); | ||
| console.log(`[MCP] Connected to server: ${config.serverId}`); | ||
| } catch (error) { | ||
| console.error(`[MCP] Connection failed for ${config.serverId}:`, error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add resource cleanup and duplicate serverId handling.
Multiple issues require attention:
- Resource leak: Replacing an existing client (same
serverId) without disconnecting the old connection leaks resources and may leave stale connections open. - No URL validation:
config.urlis passed directly toMCPClientwithout validation, risking runtime errors from malformed URLs. - Silent failures: Connection failures are logged but not propagated, preventing callers from detecting or handling errors appropriately.
🔧 Proposed fix
public async connectServer(config: MCPConfig): Promise<void> {
if (!config.enabled) return;
+
+ // Validate URL
+ try {
+ new URL(config.url);
+ } catch {
+ throw new Error(`Invalid MCP server URL: ${config.url}`);
+ }
+
+ // Disconnect existing client if present
+ const existingClient = this.clients.get(config.serverId);
+ if (existingClient) {
+ await existingClient.disconnect?.();
+ }
try {
// Initialize connection to the remote MCP server
const client = new MCPClient({ url: config.url });
await client.connect();
this.clients.set(config.serverId, client);
console.log(`[MCP] Connected to server: ${config.serverId}`);
} catch (error) {
console.error(`[MCP] Connection failed for ${config.serverId}:`, error);
+ throw error;
}
}📝 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.
| public async connectServer(config: MCPConfig): Promise<void> { | |
| if (!config.enabled) return; | |
| try { | |
| // Initialize connection to the remote MCP server | |
| const client = new MCPClient({ url: config.url }); | |
| await client.connect(); | |
| this.clients.set(config.serverId, client); | |
| console.log(`[MCP] Connected to server: ${config.serverId}`); | |
| } catch (error) { | |
| console.error(`[MCP] Connection failed for ${config.serverId}:`, error); | |
| } | |
| } | |
| public async connectServer(config: MCPConfig): Promise<void> { | |
| if (!config.enabled) return; | |
| try { | |
| new URL(config.url); | |
| } catch { | |
| throw new Error(`Invalid MCP server URL: ${config.url}`); | |
| } | |
| const existingClient = this.clients.get(config.serverId); | |
| if (existingClient) { | |
| await existingClient.disconnect?.(); | |
| } | |
| try { | |
| const client = new MCPClient({ url: config.url }); | |
| await client.connect(); | |
| this.clients.set(config.serverId, client); | |
| console.log(`[MCP] Connected to server: ${config.serverId}`); | |
| } catch (error) { | |
| console.error(`[MCP] Connection failed for ${config.serverId}:`, error); | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In @packages/core-services/src/mcp/MCPGatewayService.ts around lines 15 - 28, In
connectServer, prevent resource leaks and improve error handling by first
validating config.url (e.g., using the URL constructor) and throwing immediately
for malformed URLs; if this.clients has an existing client for config.serverId,
call its disconnect/close method (the existing MCPClient instance) and remove it
before creating a new MCPClient; attempt to connect the new MCPClient and on any
connection error ensure you clean up (disconnect/close the new client if
partially connected) and rethrow the error so callers can handle failures
instead of only logging; update references to connectServer, MCPClient,
MCPConfig, and this.clients accordingly.
| public async getAllTools() { | ||
| const allTools = []; | ||
| for (const client of this.clients.values()) { | ||
| const tools = await client.listTools(); | ||
| allTools.push(...tools); | ||
| } | ||
| return allTools; | ||
| } |
There was a problem hiding this comment.
Handle individual client failures gracefully.
If any client.listTools() call throws, the entire method fails, preventing users from accessing tools from healthy servers. One misbehaving MCP server should not block all tool discovery.
🛡️ Proposed fix for resilient tool aggregation
public async getAllTools() {
const allTools = [];
for (const client of this.clients.values()) {
- const tools = await client.listTools();
- allTools.push(...tools);
+ try {
+ const tools = await client.listTools();
+ allTools.push(...tools);
+ } catch (error) {
+ console.error('[MCP] Failed to list tools from client:', error);
+ // Continue with other clients
+ }
}
return allTools;
}🤖 Prompt for AI Agents
In @packages/core-services/src/mcp/MCPGatewayService.ts around lines 33 - 40,
getAllTools currently fails if any client.listTools() throws; update it to catch
errors per-client so one misbehaving MCP server doesn't block aggregation:
inside getAllTools iterate over this.clients.values(), call await
client.listTools() inside a try/catch, on success push results into allTools, on
error log the failure including a client-identifying symbol (e.g., client.id,
client.name or the client object) and continue to the next client, then return
the aggregated allTools as before.
| import { Component } from 'preact'; | ||
| import Router, { route } from 'preact-router'; | ||
| import { withTranslation } from 'react-i18next'; | ||
| import { ErrorBoundary } from 'react-error-boundary'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check package.json for the livechat package to see alias configuration
echo "=== Checking livechat package.json ==="
cat packages/livechat/package.json
echo -e "\n=== Looking for preact/compat configuration ==="
# Search for preact/compat references
rg -n "preact/compat" packages/livechat/ --type json --type js --type ts
echo -e "\n=== Checking for react alias configuration ==="
# Look for alias config in package.json or build files
rg -n "alias.*react|\"react\":" packages/livechat/ --type json -A2 -B2
echo -e "\n=== Check webpack or build configuration ==="
find packages/livechat -name "webpack*.js" -o -name "vite.config.*" -o -name "rollup.config.*" -o -name "tsconfig.json" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 12601
🏁 Script executed:
#!/bin/bash
# Check the root package.json for workspace configuration related to livechat
echo "=== Root monorepo config (package.json extract) ==="
rg -n "preact|alias" package.json -A2 -B2
echo -e "\n=== Check if react-error-boundary is installed ==="
rg -n "react-error-boundary" packages/livechat/package.json
echo -e "\n=== Look for any compatibility setup in livechat ==="
find packages/livechat -name ".babelrc*" -o -name "jest.config.*" | xargs cat 2>/dev/nullRepository: RocketChat/Rocket.Chat
Length of output: 223
🏁 Script executed:
#!/bin/bash
# Check the actual imports in App.tsx
cat -n packages/livechat/src/components/App/App.tsx | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1133
🏁 Script executed:
#!/bin/bash
# Check if react-error-boundary appears anywhere in dependencies across the monorepo
rg "react-error-boundary" --type json | head -20Repository: RocketChat/Rocket.Chat
Length of output: 194
🏁 Script executed:
#!/bin/bash
# Check if react-error-boundary is a transitive dependency from other packages in livechat
echo "=== Checking if react-error-boundary might be transitive ==="
# Check react-i18next and other react packages in livechat
rg "react-i18next|react-hook-form" packages/livechat/package.json -A1 -B1
echo -e "\n=== Check package-lock or yarn.lock for react-error-boundary ==="
# Look for any reference in lock files
find . -maxdepth 1 -name "*lock*" -o -name "*yarn*" | head -5
echo -e "\n=== Verify if the build actually works ==="
# Check if there are any build errors or warnings in CI config related to this
find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "livechat" 2>/dev/null | head -3Repository: RocketChat/Rocket.Chat
Length of output: 395
🏁 Script executed:
#!/bin/bash
# Simpler check: look at what react-i18next depends on
echo "=== Dependencies of packages that might pull in react-error-boundary ==="
cat packages/livechat/package.json | grep -A1 '"react-i18next"'
# Check if there's any documentation about the missing dependency
grep -r "react-error-boundary" packages/livechat --include="*.md" --include="*.ts" --include="*.tsx" --include="*.js" | grep -v node_modules | head -10Repository: RocketChat/Rocket.Chat
Length of output: 302
react-error-boundary is not installed in this package.
The import on line 6 references react-error-boundary, but this package is missing from packages/livechat/package.json entirely. While preact/compat is properly configured in webpack and TypeScript, it cannot resolve a missing package. Either add react-error-boundary to dependencies or use a Preact-compatible alternative.
🤖 Prompt for AI Agents
In @packages/livechat/src/components/App/App.tsx around lines 3 - 6, The file
imports ErrorBoundary from 'react-error-boundary' in App.tsx but the package is
not listed in packages/livechat/package.json; either add "react-error-boundary"
to the livechat package dependencies and run the package manager install, or
replace the usage/import of ErrorBoundary in App.tsx with a Preact-compatible
alternative (e.g., implement a small Preact ErrorBoundary component or use a
preact-compatible library), updating the import and any usage sites accordingly
so the module resolution succeeds.
AI Review Feedback ResolvedI have addressed the violations identified by @cubic-dev-ai and @coderabbitai across the Livechat and MCP components. Changes:
@cubic-dev-ai re-run |
@Kalpana-chavhan I have started the AI code review. It will take a few minutes to complete. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6202 @Kalpana-chavhan, great work on the type safety improvements in ✅ Fully AddressedType Safety & Defensive Coding in
|
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
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/ui-client/package.json">
<violation number="1" location="packages/ui-client/package.json:2">
P0: Critical: This package is being renamed from `@rocket.chat/ui-client` to `@rocket.chat/livechat`, but `@rocket.chat/livechat` already exists in `packages/livechat/`. This creates a name collision that will break workspace resolution. This change also appears unrelated to the PR's stated purpose (MCP Gateway integration) and may be accidental.</violation>
<violation number="2" location="packages/ui-client/package.json:22">
P0: Adding `@rocket.chat/ui-client` as a workspace dependency is problematic since this same file's name is being changed from `@rocket.chat/ui-client`. This creates an unresolvable dependency. If the name change is reverted, this dependency line should also be removed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "dependencies": { | ||
| "@rocket.chat/onboarding-ui": "~0.36.0", | ||
| "dompurify": "~3.2.7" | ||
| "@rocket.chat/ui-client": "workspace:~", |
There was a problem hiding this comment.
P0: Adding @rocket.chat/ui-client as a workspace dependency is problematic since this same file's name is being changed from @rocket.chat/ui-client. This creates an unresolvable dependency. If the name change is reverted, this dependency line should also be removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ui-client/package.json, line 22:
<comment>Adding `@rocket.chat/ui-client` as a workspace dependency is problematic since this same file's name is being changed from `@rocket.chat/ui-client`. This creates an unresolvable dependency. If the name change is reverted, this dependency line should also be removed.</comment>
<file context>
@@ -19,7 +19,9 @@
"dependencies": {
"@rocket.chat/onboarding-ui": "~0.36.0",
- "dompurify": "~3.2.7"
+ "@rocket.chat/ui-client": "workspace:~",
+ "dompurify": "~3.2.7",
+ "react-error-boundary": "^5.0.0"
</file context>
| { | ||
| "name": "@rocket.chat/ui-client", | ||
| "version": "26.0.0-rc.1", | ||
| "name": "@rocket.chat/livechat", |
There was a problem hiding this comment.
P0: Critical: This package is being renamed from @rocket.chat/ui-client to @rocket.chat/livechat, but @rocket.chat/livechat already exists in packages/livechat/. This creates a name collision that will break workspace resolution. This change also appears unrelated to the PR's stated purpose (MCP Gateway integration) and may be accidental.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ui-client/package.json, line 2:
<comment>Critical: This package is being renamed from `@rocket.chat/ui-client` to `@rocket.chat/livechat`, but `@rocket.chat/livechat` already exists in `packages/livechat/`. This creates a name collision that will break workspace resolution. This change also appears unrelated to the PR's stated purpose (MCP Gateway integration) and may be accidental.</comment>
<file context>
@@ -1,6 +1,6 @@
{
- "name": "@rocket.chat/ui-client",
- "version": "26.0.0-rc.1",
+ "name": "@rocket.chat/livechat",
+ "version": "1.0.0",
"private": true,
</file context>
| "name": "@rocket.chat/livechat", | |
| "name": "@rocket.chat/ui-client", |
There was a problem hiding this comment.
1 issue found across 10 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/ui-client/package.json">
<violation number="1" location="packages/ui-client/package.json:2">
P0: Critical: Package name incorrectly changed from `@rocket.chat/ui-client` to `@rocket.chat/livechat`. This name is already used by `packages/livechat/package.json`, creating a duplicate. This appears to be an accidental merge conflict or copy-paste error that will break the monorepo.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| { | ||
| "name": "@rocket.chat/ui-client", | ||
| "version": "26.0.0-rc.1", | ||
| "name": "@rocket.chat/livechat", |
There was a problem hiding this comment.
P0: Critical: Package name incorrectly changed from @rocket.chat/ui-client to @rocket.chat/livechat. This name is already used by packages/livechat/package.json, creating a duplicate. This appears to be an accidental merge conflict or copy-paste error that will break the monorepo.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ui-client/package.json, line 2:
<comment>Critical: Package name incorrectly changed from `@rocket.chat/ui-client` to `@rocket.chat/livechat`. This name is already used by `packages/livechat/package.json`, creating a duplicate. This appears to be an accidental merge conflict or copy-paste error that will break the monorepo.</comment>
<file context>
@@ -1,6 +1,6 @@
{
- "name": "@rocket.chat/ui-client",
- "version": "26.0.0-rc.1",
+ "name": "@rocket.chat/livechat",
+ "version": "1.0.0",
"private": true,
</file context>
| "name": "@rocket.chat/livechat", | |
| "name": "@rocket.chat/ui-client", |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @apps/meteor/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsx:
- Line 5: The file contains stale "FIX" inline comments (e.g., in
MCPToolList.tsx near the MCPToolList component) that violate coding guidelines
and reference an old any[] issue; remove all such FIX comments (the ones noted
around lines 5, 18, 33, 37) from the source so the implementation contains no
FIXME/FIX comments while leaving the surrounding code and type definitions
(props, interfaces, and any existing types used by MCPToolList and its helper
functions) unchanged.
- Line 3: Replace the direct react-i18next import with the project's translation
hook: in MCPToolList.tsx stop importing useTranslation from 'react-i18next' and
import the useTranslation provided by the project (from
'@rocket.chat/ui-contexts') so the MCPToolList component uses the app's i18n
context; update any references to useTranslation in that file accordingly.
- Line 38: The nullish-coalescing fallback t('Unknown_Tool') in the Box
rendering is dead code because validation earlier guarantees tool.name is a
valid non-empty string; remove the unreachable fallback and render tool.name
directly (replace "tool.name ?? t('Unknown_Tool')" with "tool.name") in
MCPToolList.tsx to keep the JSX concise and avoid misleading code paths.
In @packages/ui-client/package.json:
- Line 59: The package.json contains an unused devDependency "preact"; remove
the "preact" entry from devDependencies in package.json, run the project package
manager to update the lockfile (npm install / yarn install / pnpm install as
appropriate), and verify there are no imports or build/test references to
"preact" (grep for "preact" or search IDE-wide) before committing the change.
- Around line 2-3: packages/ui-client's package.json was renamed to
"@rocket.chat/livechat", creating a duplicate package name clash with
packages/livechat and leaving dependencies pointing to the old name; restore a
single authoritative package name and update dependent packages accordingly:
either revert packages/ui-client's "name" to "@rocket.chat/ui-client" (or
remove/merge that package if it should be consolidated into packages/livechat),
then update all package.json dependency entries that reference
"@rocket.chat/ui-client" (including in packages/ui-voip,
packages/web-ui-registration, packages/gazzodown, and apps/meteor) to the final
chosen name ("@rocket.chat/livechat" if you consolidated, or
"@rocket.chat/ui-client" if you kept them separate) and ensure workspace:
versions match the package version in the monorepo; finally remove/resolve the
duplicate export by keeping only one package with the chosen name and run the
package manager's workspace resolution check to confirm no remaining conflicts.
- Around line 23-24: The package.json dependencies for the ui-client list older
versions of dompurify and react-error-boundary; update "dompurify" from "~3.2.7"
to "3.3.1" (or a compatible ^3.3.1 range) and "react-error-boundary" from
"^5.0.0" to the current stable (e.g., "^6.0.2") in the dependencies section,
then run your package manager (npm/yarn/pnpm) to refresh lockfiles and test the
app to ensure no breaking changes from the upgraded versions affect components
that import dompurify or the ErrorBoundary from react-error-boundary.
📜 Review details
Configuration used: Organization 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 (3)
apps/meteor/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsxpackages/livechat/src/components/App/App.tsxpackages/ui-client/package.json
🧰 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/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsxpackages/livechat/src/components/App/App.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-10T21:00:54.909Z
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.
Applied to files:
packages/livechat/src/components/App/App.tsx
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/livechat/src/components/App/App.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsx (2)
packages/ui-contexts/src/index.ts (1)
useTranslation(81-81)packages/ui-kit/src/index.ts (1)
Option(2-2)
packages/livechat/src/components/App/App.tsx (2)
packages/ui-client/src/components/errors/ErrorFallbacks.tsx (1)
GlobalErrorFallback(6-20)packages/ui-client/src/services/ErrorTrackingService.ts (1)
errorTrackingService(25-25)
🪛 Biome (2.1.2)
packages/livechat/src/components/App/App.tsx
[error] 30-30: Shouldn't redeclare 'ScreenProvider'. Consider to delete it or rename it.
'ScreenProvider' is defined here:
(lint/suspicious/noRedeclare)
[error] 31-31: Shouldn't redeclare 'GlobalErrorFallback'. Consider to delete it or rename it.
'GlobalErrorFallback' is defined here:
(lint/suspicious/noRedeclare)
[error] 31-31: Shouldn't redeclare 'errorTrackingService'. Consider to delete it or rename it.
'errorTrackingService' is defined here:
(lint/suspicious/noRedeclare)
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (3)
packages/livechat/src/components/App/App.tsx (2)
6-6: Remove duplicate imports.Lines 28-30 duplicate imports already declared on line 27 and line 31. The barrel export from
@rocket.chat/ui-clienton line 31 supersedes the relative path imports on lines 28-29, andScreenProvideris redundantly imported on line 30 after being imported on line 27.🐛 Proposed fix
-import { GlobalErrorFallback } from '../../../../ui-client/src/components/errors/ErrorFallbacks'; -import { errorTrackingService } from '../../../../ui-client/src/services/ErrorTrackingService'; -import { ScreenProvider } from '../Screen/ScreenProvider'; import { GlobalErrorFallback, errorTrackingService } from '@rocket.chat/ui-client';Also applies to: 28-31
⛔ Skipped due to learnings
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.
210-230: ErrorBoundary implementation is correct and compatible.React 18.3.1 is used throughout the codebase, not React 19, and react-error-boundary ^5.0.0 is confirmed compatible with React 18.3.1. The ErrorBoundary configuration with appropriate error tracking metadata for global scope requires no changes.
packages/ui-client/package.json (1)
22-22: Fix the package name inpackages/ui-client/package.json.The package currently declares
"name": "@rocket.chat/livechat"(line 1), which is the same name as the package inpackages/livechat/. This creates a duplicate in the workspace. Additionally, the dependency on"@rocket.chat/ui-client"(line 22) references a non-existent package since no package is actually named@rocket.chat/ui-client.Change line 1 in
packages/ui-client/package.jsonfrom:"name": "@rocket.chat/livechat",to:
"name": "@rocket.chat/ui-client",This will resolve the duplicate package name conflict and make the workspace dependency resolvable.
⛔ Skipped due to learnings
Learnt from: d-gubert Repo: RocketChat/Rocket.Chat PR: 37547 File: packages/i18n/src/locales/en.i18n.json:634-634 Timestamp: 2025-11-19T12:32:29.696Z Learning: Repo: RocketChat/Rocket.Chat Context: i18n workflow Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.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.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: gabriellsh Repo: RocketChat/Rocket.Chat PR: 37419 File: packages/i18n/src/locales/en.i18n.json:918-921 Timestamp: 2025-11-19T18:20:07.720Z Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
| @@ -0,0 +1,47 @@ | |||
| import { Box, Option, Icon, States, StatesIcon, StatesTitle } from '@rocket.chat/fuselage'; | |||
| import React from 'react'; | |||
| import { useTranslation } from 'react-i18next'; | |||
There was a problem hiding this comment.
Use the project's standard useTranslation hook.
The component imports useTranslation from react-i18next, but the project uses its own implementation from @rocket.chat/ui-contexts. This inconsistency could cause i18n to fail or use an incorrect translation context.
🔧 Proposed fix
-import { useTranslation } from 'react-i18next';
+import { useTranslation } from '@rocket.chat/ui-contexts';📝 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.
| import { useTranslation } from 'react-i18next'; | |
| import { useTranslation } from '@rocket.chat/ui-contexts'; |
🤖 Prompt for AI Agents
In @apps/meteor/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsx at line
3, Replace the direct react-i18next import with the project's translation hook:
in MCPToolList.tsx stop importing useTranslation from 'react-i18next' and import
the useTranslation provided by the project (from '@rocket.chat/ui-contexts') so
the MCPToolList component uses the app's i18n context; update any references to
useTranslation in that file accordingly.
| import React from 'react'; | ||
| import { useTranslation } from 'react-i18next'; | ||
|
|
||
| // FIX: Define proper interface to replace 'any[]' |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove FIX comments per coding guidelines.
The coding guidelines specify to avoid code comments in the implementation. The FIX comment on line 5 is also stale, referencing any[] that no longer exists in the code.
♻️ Proposed refactor
-// FIX: Define proper interface to replace 'any[]'
interface MCPTool {
name: string;
description: string;
}- // FIX: Defensive validation - filter out malformed data
const validTools = tools.filter((tool) => tool && typeof tool.name === 'string' && tool.name.trim() !== '');- // FIX: Use a unique key combining name and index for stability
<Option key={`${tool.name}-${index}`}> <Box mi='x8'>
- {/* FIX: Use nullish coalescing for display safety */}
<Box fontScale='p2m'>{tool.name ?? t('Unknown_Tool')}</Box>Also applies to: 18-18, 33-33, 37-37
🤖 Prompt for AI Agents
In @apps/meteor/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsx at line
5, The file contains stale "FIX" inline comments (e.g., in MCPToolList.tsx near
the MCPToolList component) that violate coding guidelines and reference an old
any[] issue; remove all such FIX comments (the ones noted around lines 5, 18,
33, 37) from the source so the implementation contains no FIXME/FIX comments
while leaving the surrounding code and type definitions (props, interfaces, and
any existing types used by MCPToolList and its helper functions) unchanged.
| <Icon name='robot' size='x20' /> | ||
| <Box mi='x8'> | ||
| {/* FIX: Use nullish coalescing for display safety */} | ||
| <Box fontScale='p2m'>{tool.name ?? t('Unknown_Tool')}</Box> |
There was a problem hiding this comment.
Remove unreachable fallback code.
The validation on line 19 ensures tool.name is always a valid non-empty string. The nullish coalescing fallback t('Unknown_Tool') on line 38 is unreachable dead code.
🧹 Proposed fix
- <Box fontScale='p2m'>{tool.name ?? t('Unknown_Tool')}</Box>
+ <Box fontScale='p2m'>{tool.name}</Box>📝 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.
| <Box fontScale='p2m'>{tool.name ?? t('Unknown_Tool')}</Box> | |
| <Box fontScale='p2m'>{tool.name}</Box> |
🤖 Prompt for AI Agents
In @apps/meteor/client/views/room/contextualBar/OTR/MCP/MCPToolList.tsx at line
38, The nullish-coalescing fallback t('Unknown_Tool') in the Box rendering is
dead code because validation earlier guarantees tool.name is a valid non-empty
string; remove the unreachable fallback and render tool.name directly (replace
"tool.name ?? t('Unknown_Tool')" with "tool.name") in MCPToolList.tsx to keep
the JSX concise and avoid misleading code paths.
| "name": "@rocket.chat/livechat", | ||
| "version": "1.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all references to the old package name @rocket.chat/ui-client
echo "=== Searching for import statements referencing @rocket.chat/ui-client ==="
rg -n '@rocket\.chat/ui-client' --type ts --type tsx --type js --type jsx -g '!node_modules' -g '!dist' -g '!*.lock' 2>/dev/null | head -50
echo ""
echo "=== Searching in all package.json files for @rocket.chat/ui-client ==="
fd -e json 'package.json' -x grep -H '@rocket\.chat/ui-client' {} \; | grep -v node_modules | head -30
echo ""
echo "=== Checking current directory structure ==="
ls -la packages/ | grep -E "(ui-client|livechat)"
echo ""
echo "=== Checking for references to @rocket.chat/livechat ==="
rg -n '@rocket\.chat/livechat' --type ts --type tsx --type js --type jsx -g '!node_modules' -g '!dist' 2>/dev/null | head -30
echo ""
echo "=== List all package.json files in packages directory ==="
fd 'package.json' packages/ | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1690
🏁 Script executed:
#!/bin/bash
echo "=== packages/ui-client/package.json content ==="
cat packages/ui-client/package.json
echo ""
echo "=== packages/livechat/package.json content ==="
cat packages/livechat/package.jsonRepository: RocketChat/Rocket.Chat
Length of output: 6840
Critical: Duplicate package name and broken dependencies prevent build.
The package in packages/ui-client is being renamed from @rocket.chat/ui-client to @rocket.chat/livechat, but this creates two critical issues:
-
Duplicate package name conflict: Both
packages/ui-client(v1.0.0) andpackages/livechat(v2.0.0-rc.1) now export@rocket.chat/livechat. This is invalid in a monorepo and will cause resolution failures. -
Broken dependencies: The package depends on
@rocket.chat/ui-client("workspace:~") which no longer exists (renamed to@rocket.chat/livechat). Additionally, 4+ packages still depend on the old@rocket.chat/ui-clientname and have not been updated:packages/ui-voippackages/web-ui-registrationpackages/gazzodownapps/meteor
All references must be updated consistently, and the duplicate package name must be resolved before this can merge.
🤖 Prompt for AI Agents
In @packages/ui-client/package.json around lines 2 - 3, packages/ui-client's
package.json was renamed to "@rocket.chat/livechat", creating a duplicate
package name clash with packages/livechat and leaving dependencies pointing to
the old name; restore a single authoritative package name and update dependent
packages accordingly: either revert packages/ui-client's "name" to
"@rocket.chat/ui-client" (or remove/merge that package if it should be
consolidated into packages/livechat), then update all package.json dependency
entries that reference "@rocket.chat/ui-client" (including in packages/ui-voip,
packages/web-ui-registration, packages/gazzodown, and apps/meteor) to the final
chosen name ("@rocket.chat/livechat" if you consolidated, or
"@rocket.chat/ui-client" if you kept them separate) and ensure workspace:
versions match the package version in the monorepo; finally remove/resolve the
duplicate export by keeping only one package with the chosen name and run the
package manager's workspace resolution check to confirm no remaining conflicts.
| "dompurify": "~3.2.7", | ||
| "react-error-boundary": "^5.0.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
react-error-boundary npm latest version 2025 security vulnerabilities
💡 Result:
Short answer (as of Jan 8, 2026): the react-error-boundary package latest published version is 6.0.1, and there are no known direct security vulnerabilities reported for the package itself. [1][2]
Sources:
- Snyk package/security page for react-error-boundary (shows latest 6.0.1, no direct vulnerabilities). [1]
- jsDelivr / package metadata showing version 6.0.1. [2]
If you want, I can list its release date, scan it for transitive (dependency) issues, or check npm advisories/CVE databases for further confirmation.
🌐 Web query:
dompurify npm latest version 2025 security vulnerabilities
💡 Result:
Short answer
- Latest npm release (as of Jan 8, 2026): DOMPurify v3.2.4. [1]
Known security issues (important ones from 2024–2025)
- CVE-2024-45801 — prototype-pollution / nesting bypass (fixed in 2.5.4 and 3.1.3). [2]
- CVE-2024-47875 — nesting-based mutation XSS (mXSS) (fixed in 2.5.0 and 3.1.3). [3]
- CVE-2025-26791 — incorrect template-literal regex when SAFE_FOR_TEMPLATES=true leading to mXSS (fixed in 3.2.4). [4]
Recommendation
- Upgrade to v3.2.4 (or newer) and audit usages of SAFE_FOR_TEMPLATES; follow project/security advisories for patches. [1][4]
Sources
[1] npm package / DOMPurify release info.
[2] NVD — CVE-2024-45801.
[3] NVD — CVE-2024-47875.
[4] CVE-2025-26791 advisory / DOMPurify 3.2.4 release.
🏁 Script executed:
#!/bin/bash
# Check npm registry data for both packages to get version info and vulnerability status
npm view react-error-boundary@5.0.0 --json 2>/dev/null | head -50
npm view dompurify@3.2.7 --json 2>/dev/null | head -50Repository: RocketChat/Rocket.Chat
Length of output: 1506
Consider updating to more current versions.
The versions specified are functional but outdated:
react-error-boundary@5.0.0is 1–2 major versions behind the latest available (6.0.2). No known security vulnerabilities in 5.0.0 itself.dompurify@3.2.7is slightly behind the latest (3.3.1). The version includes fixes for critical CVEs like CVE-2025-26791 (mXSS with SAFE_FOR_TEMPLATES), so it is secure, but updating to 3.3.1 would be prudent for long-term maintainability.
🤖 Prompt for AI Agents
In @packages/ui-client/package.json around lines 23 - 24, The package.json
dependencies for the ui-client list older versions of dompurify and
react-error-boundary; update "dompurify" from "~3.2.7" to "3.3.1" (or a
compatible ^3.3.1 range) and "react-error-boundary" from "^5.0.0" to the current
stable (e.g., "^6.0.2") in the dependencies section, then run your package
manager (npm/yarn/pnpm) to refresh lockfiles and test the app to ensure no
breaking changes from the upgraded versions affect components that import
dompurify or the ErrorBoundary from react-error-boundary.
| "jest": "~30.2.0", | ||
| "overlayscrollbars": "^2.11.4", | ||
| "overlayscrollbars-react": "^0.5.6", | ||
| "preact": "^10.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for preact usage in the codebase
echo "=== Searching for preact imports ==="
rg -n "from ['\"]preact" packages/ui-client/ -g '!node_modules' -g '!dist' 2>/dev/null || echo "No matches found"
echo ""
echo "=== Searching for preact/compat usage ==="
rg -n "preact/compat" packages/ui-client/ -g '!node_modules' -g '!dist' 2>/dev/null || echo "No matches found"
echo ""
echo "=== Searching for preact in require statements ==="
rg -n "require.*preact" packages/ui-client/ -g '!node_modules' -g '!dist' 2>/dev/null || echo "No matches found"
echo ""
echo "=== Checking package.json for preact references ==="
cat packages/ui-client/package.json | grep -i preact || echo "No preact references in package.json"
echo ""
echo "=== Checking jest/webpack/vite configs ==="
fd -t f \( -name 'jest.config.*' -o -name 'webpack.config.*' -o -name 'vite.config.*' \) packages/ui-client/ 2>/dev/null | head -5 | xargs -I {} sh -c 'echo "File: {}"; grep -i preact {} 2>/dev/null || echo " (no preact references)"'Repository: RocketChat/Rocket.Chat
Length of output: 368
Remove the unused preact devDependency.
The preact dependency is not imported or used anywhere in the codebase and is not referenced in any build or test configurations. It should be removed from package.json.
🤖 Prompt for AI Agents
In @packages/ui-client/package.json at line 59, The package.json contains an
unused devDependency "preact"; remove the "preact" entry from devDependencies in
package.json, run the project package manager to update the lockfile (npm
install / yarn install / pnpm install as appropriate), and verify there are no
imports or build/test references to "preact" (grep for "preact" or search
IDE-wide) before committing the change.
1 participant
Proposed Changes
This PR introduces the foundational architecture for Model Context Protocol (MCP) integration within Rocket.Chat. By implementing an MCP Gateway, Rocket.Chat can now act as a Host, allowing users to connect third-party AI Tool Servers (such as those proposed in #38092) directly into their chat environment.
Key Implementations:
packages/core-services):Why this approach?
Instead of hard-coding AI features, this integration follows the Anthropic MCP standard, making Rocket.Chat "future-proof." It enables Rocket.Chat to bridge the gap between team communication and AI-powered productivity (automated task management, content synthesis, etc.).
Issue(s)
Closes #38092
Steps to test or reproduce
get_task,create_summary) are correctly listed with their descriptions.listToolsJSON-RPC handshake is completed successfully.Checklist
Further comments
This is Phase 1 of the integration. Phase 2 will involve a "Tool Executor" that allows AI agents to actually run these tools based on user prompts.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.