[BUG][Chat] Unregister tools when component unmount#10999
[BUG][Chat] Unregister tools when component unmount#10999ananzh merged 2 commits intoopensearch-project:mainfrom
Conversation
ananzh
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
Maosaic,
SuZhou-Joe,
TackAdam,
ZilongX,
abbyhu2000,
angle943,
ashwin-pc,
bandinib-amzn,
curq,
d-buckner,
huyaboo,
joshuali925,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
ps48,
raintygao,
ruanyl,
sejli,
virajsanghvi,
wanglam,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
WalkthroughIntroduces a core Chat Service architecture by extracting chat functionality into a managed CoreService with lifecycle control, thread/window state management, and plugin delegation. Integrates chat throughout the core system and refactors existing chat and explore plugins to use the new service. Removes legacy graph visualization error handling and adds ML agent routing infrastructure. Changes
Sequence DiagramsequenceDiagram
participant Plugin as Chat Plugin
participant Core as CoreSystem
participant ChatSvc as ChatService (Core)
participant AppImpl as ChatImplementation (Plugin)
participant UI as UI/Components
Note over Core,ChatSvc: Initialization Phase
Core->>ChatSvc: new ChatService()
ChatSvc->>ChatSvc: Initialize threadId$, windowState$
Core->>ChatSvc: setup()
ChatSvc-->>Core: ChatServiceSetup { setImplementation, ... }
Core-->>Plugin: expose in CoreSetup
Plugin->>Plugin: initialize ChatService impl
Plugin->>Core: setup(core)
Plugin->>ChatSvc: setImplementation(chatImpl)
Note over Core,UI: Start Phase
Core->>ChatSvc: start()
ChatSvc-->>Core: ChatServiceStart { isAvailable, sendMessage, ... }
Core-->>Plugin: expose in CoreStart
Plugin->>ChatSvc: ChatService(uiSettings, coreChatService)
Note over ChatSvc,UI: Message Send Flow
UI->>Plugin: sendMessage(content)
Plugin->>ChatSvc: sendMessage(content)
ChatSvc->>ChatSvc: validate availability via impl
ChatSvc->>AppImpl: delegated to implementation
AppImpl-->>ChatSvc: Observable<Message>
ChatSvc-->>Plugin: Observable streams
Plugin-->>UI: message updates
Note over ChatSvc,AppImpl: Window Management
UI->>ChatSvc: openWindow()
ChatSvc->>AppImpl: delegated openWindow
AppImpl-->>ChatSvc: success
ChatSvc->>ChatSvc: updateWindowState$
ChatSvc->>UI: notify onWindowOpen callbacks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/chat/public/services/chat_service.ts (1)
446-457:saveCurrentChatStatecallsthis.getThreadId()which may throw.If the core chat service is unavailable when
saveCurrentChatStateis called (e.g., during early initialization or cleanup), this will throw an error. Consider adding a guard.private saveCurrentChatState(): void { + if (!this.coreChatService) { + console.warn('Cannot save chat state: core chat service not available'); + return; + } const state: CurrentChatState = { threadId: this.getThreadId(), messages: this.currentMessages, };
🧹 Nitpick comments (23)
src/plugins/chat/public/actions/graph_timeseries_data_action.tsx (1)
42-42: LGTM! Clean API extension with backward compatibility.The addition of the
enabledparameter with a default value oftruemaintains backward compatibility while enabling runtime control of action registration.Optionally, consider adding JSDoc documentation to clarify the parameter's purpose:
+/** + * Registers the graph timeseries data action with the assistant. + * @param enabled - Controls whether the action is enabled (default: true) + */ export function useGraphTimeseriesDataAction(enabled: boolean = true) {src/plugins/chat/common/types.ts (1)
8-20: Core chat type re-exports are a clean consolidationRe-exporting ToolCall/FunctionCall/Message/Role from
../../../core/public/chatcentralizes the structural definitions while keeping local Zod schemas for runtime validation. This reduces duplication of interface declarations.src/plugins/chat/public/services/ag_ui_agent.ts (1)
31-53: OptionaldataSourceIdsupport is good; consider more robust URL buildingExtending
runAgentwith an optionaldataSourceIdand threading it via a query parameter is backward compatible and solves the routing need.To make this future‑proof if
proxyUrlever includes its own query string, consider usingURL/URLSearchParamsor at least switching between?and&based on the presence of?:- const url = dataSourceId - ? `${this.proxyUrl}?dataSourceId=${encodeURIComponent(dataSourceId)}` - : this.proxyUrl; + const url = (() => { + if (!dataSourceId) return this.proxyUrl; + const separator = this.proxyUrl.includes('?') ? '&' : '?'; + return `${this.proxyUrl}${separator}dataSourceId=${encodeURIComponent(dataSourceId)}`; + })();src/plugins/explore/public/actions/ask_ai_action.test.ts (1)
15-35: Avoid hand‑rolled ChatServiceStart mocks; reuse the shared core mockThe locally constructed
jest.Mocked<ChatServiceStart>only covers part of theChatServiceStartsurface and will easily drift as the core chat API evolves. We already havecoreChatServiceMock.createStartContract()providing a fully‑typed, up‑to‑date mock.Consider using the shared mock instead, and just overwrite
isAvailablefor specific scenarios, e.g.:const mockChatService = coreChatServiceMock.createStartContract(); beforeEach(() => { jest.clearAllMocks(); mockChatService.isAvailable.mockReturnValue(true); }); it('should return false when chatService is not available', () => { mockChatService.isAvailable.mockReturnValue(false); const action = createAskAiAction(mockChatService); expect(action.isCompatible(mockContext)).toBe(false); });This keeps tests aligned with the real
ChatServiceStartcontract and avoids missing fields like future window/thread APIs.Also applies to: 56-68, 72-91
src/core/public/chat/chat_service.mock.ts (1)
15-48: Align mock observables with real ChatService behavior
getThreadId$andgetWindowState$currently create a newBehaviorSubjecton every invocation. The realChatServiceexposes a single subject per instance, and repeated calls toget*$( )return observables backed by the same internal subject.To better mirror production behavior (and avoid surprising tests that expect shared state), consider creating the subjects once per mock instance and reusing them:
-import { BehaviorSubject } from 'rxjs'; -import { ChatServiceSetup, ChatServiceStart } from './types'; +import { BehaviorSubject } from 'rxjs'; +import { ChatServiceSetup, ChatServiceStart, ChatWindowState } from './types'; const createStartContractMock = (): jest.Mocked<ChatServiceStart> => ({ - isAvailable: jest.fn().mockReturnValue(false), - isWindowOpen: jest.fn().mockReturnValue(false), - getThreadId$: jest.fn().mockReturnValue(new BehaviorSubject<string>('')), - getThreadId: jest.fn().mockReturnValue(''), + const threadId$ = new BehaviorSubject<string>(''); + const windowState$ = new BehaviorSubject<ChatWindowState>({ + isWindowOpen: false, + windowMode: 'sidecar', + paddingSize: 400, + }); + + return { + isAvailable: jest.fn().mockReturnValue(false), + isWindowOpen: jest.fn().mockReturnValue(false), + getThreadId$: jest.fn().mockReturnValue(threadId$.asObservable()), + getThreadId: jest.fn().mockReturnValue(''), setThreadId: jest.fn(), newThread: jest.fn(), openWindow: jest.fn(), closeWindow: jest.fn(), - getWindowState: jest.fn().mockReturnValue({ - isWindowOpen: false, - windowMode: 'sidecar', - paddingSize: 400, - }), - setWindowState: jest.fn(), + getWindowState: jest.fn().mockReturnValue(windowState$.getValue()), + setWindowState: jest.fn(), // ... - getWindowState$: jest.fn().mockReturnValue( - new BehaviorSubject({ - isWindowOpen: false, - windowMode: 'sidecar', - paddingSize: 400, - }) - ), + getWindowState$: jest.fn().mockReturnValue(windowState$.asObservable()), // ... - suggestedActionsService: undefined, -}); + suggestedActionsService: undefined, +});This keeps the mock semantically closer to the real service while still being easy to override in tests.
src/core/public/chat/chat_service.ts (1)
39-51: MakesuggestedActionsServiceon setup dynamically reflect later registrationIn
setup(), the returnedsuggestedActionsServiceis a plain property set to the value ofthis.suggestedActionsServiceat setup time:return { setImplementation: ..., setSuggestedActionsService: ..., suggestedActionsService: this.suggestedActionsService, };If
setSuggestedActionsServiceis invoked after a consumer has captured this setup contract, that consumer will continue to see the old (likelyundefined) snapshot.To keep setup consumers in sync with later registration, consider exposing a getter (similar to
start()):public setup(): ChatServiceSetup { - return { - setImplementation: (implementation: ChatImplementationFunctions) => { - this.implementation = implementation; - }, - - setSuggestedActionsService: (service: { registerProvider(provider: any): void }) => { - this.suggestedActionsService = service; - }, - - suggestedActionsService: this.suggestedActionsService, - }; + const chatServiceInstance = this; + return { + setImplementation: (implementation: ChatImplementationFunctions) => { + this.implementation = implementation; + }, + + setSuggestedActionsService: (service: { registerProvider(provider: any): void }) => { + this.suggestedActionsService = service; + }, + + get suggestedActionsService() { + return chatServiceInstance.suggestedActionsService; + }, + }; }That keeps the setup contract aligned with the current service state without changing the public type.
src/core/public/chat/index.ts (1)
1-24: LGTM!Clean barrel file that consolidates the chat module's public API. The exports are well-organized with types first, then implementation, then mocks.
One minor consideration: exporting
coreChatServiceMockfrom the main barrel couples test utilities with production code. Consider whether a separate./chat/mocksor./chat/index.mock.tsentry point would be more appropriate for your project conventions.src/plugins/chat/server/routes/ml_routes/router_registry.ts (1)
23-34: Clarify "initialized" semantics—flag is set even without router registration.The
initializedflag is set totrueregardless of whether a router was actually registered (i.e., whenhasInvestigationCapabilitiesreturnsfalse). This prevents re-initialization but may be misleading since "initialized" doesn't guarantee a router is available.Consider whether
isInitialized()accurately conveys the intended meaning, or if a separatehasRouter()method would be useful for callers needing to check router availability.src/plugins/chat/server/routes/ml_routes/ml_agent_router.ts (1)
56-58:registerRoutersilently replaces any existing router.If a router is already registered,
registerRouterwill overwrite it without warning. This may be intentional for simplicity, but consider whether:
- A warning log should be emitted when replacing an existing router
- An error should be thrown to prevent accidental overwrites
If the current behavior is intentional (e.g., for test substitution), a brief comment would clarify this design choice.
static registerRouter(router: MLAgentRouter): void { + // Note: Overwrites any previously registered router. This is intentional + // to allow router replacement in tests or reconfiguration scenarios. this.router = router; }src/plugins/chat/server/routes/index.test.ts (3)
251-294: Consider adding MLAgentRouterRegistry.reset() in test teardown.Based on the relevant code snippets,
MLAgentRouterRegistryhas aninitializedflag that persists. SinceMLAgentRouterRegistry.initialize()is called within the route handler per request, and tests run sequentially, state may leak between tests if the registry was initialized in a prior test.Add cleanup in
afterEach:afterEach(async () => { if (server) { await server.stop(); } + // Reset ML router registry to prevent state leakage between tests + const { MLAgentRouterRegistry } = await import('./ml_routes/router_registry'); + MLAgentRouterRegistry.reset(); });
296-339: Test case duplicates logic from previous test.The test "should fallback to AG-UI when agenticFeaturesEnabled is true but ML client is disabled" has the same setup and assertions as the prior test. The comment says "ML client disabled via missing context ML client" but there's no actual difference in the mock configuration.
Consider either differentiating the test setup or consolidating these tests.
378-403: Test title vs. behavior mismatch.The test title says "should return 503 when ML Commons agent ID is not configured" but the actual expectation is for the general "No AI agent available" message which occurs because neither ML router nor AG-UI URL is available. The
mlCommonsAgentIdbeing undefined doesn't directly cause the 503—it's the combination of no ML router and no AG-UI URL.Consider updating the test title for clarity:
- it('should return 503 when ML Commons agent ID is not configured', async () => { + it('should return 503 when neither ML router nor AG-UI URL is available', async () => {src/plugins/chat/common/chat_capabilities.ts (1)
26-28: Consider typing thecapabilitiesparameter.Using
anytype loses type safety. If aCapabilitiestype exists in the codebase (it's imported inroutes/index.ts), consider using it here for consistency.+import { Capabilities } from '../../../../core/server'; + -export function hasInvestigationCapabilities(capabilities?: any): boolean { +export function hasInvestigationCapabilities(capabilities?: Capabilities): boolean { return capabilities?.investigation?.agenticFeaturesEnabled === true; }src/plugins/chat/server/routes/index.ts (1)
51-66: Consider handling null response body defensively.Line 52 uses a non-null assertion (
agUiResponse.body!). While the body should exist after a successful fetch withok: true, some edge cases (e.g., certain proxies or unusual responses) could result in a null body.+ if (!agUiResponse.body) { + return response.customError({ + statusCode: 502, + body: { message: 'AG-UI server returned empty response body' }, + }); + } + // Convert Web ReadableStream to Node.js Readable stream - const reader = agUiResponse.body!.getReader(); + const reader = agUiResponse.body.getReader();src/plugins/chat/server/routes/ml_routes/generic_ml_router.ts (3)
12-12: Remove unused import.
Capabilitiesis imported but never used in this file.import { Logger, RequestHandlerContext, OpenSearchDashboardsRequest, IOpenSearchDashboardsResponse, OpenSearchDashboardsResponseFactory, - Capabilities, } from '../../../../../core/server';
155-160: Fragile error detection via string matching.Checking
error.message.includes('404')is unreliable—error messages can vary by locale, library version, or upstream changes. Consider checking the error'sstatusCodeproperty if available, or wrapping the ML client call to capture HTTP status codes directly.- if (error instanceof Error && error.message.includes('404')) { + // Check for statusCode property if the error exposes it + const statusCode = (error as any)?.statusCode ?? (error as any)?.status; + if (statusCode === 404) { return response.customError({ statusCode: 404, body: { message: `ML Commons agent "${configuredAgentId}" not found` }, }); }
149-149: Wrap JSON.parse in try-catch to handle malformed responses.If
mlResponse.bodyis not valid JSON,JSON.parsewill throw an uncaught exception, resulting in a 500 error with no useful message.- body: typeof mlResponse.body === 'string' ? JSON.parse(mlResponse.body) : mlResponse.body, + body: (() => { + if (typeof mlResponse.body !== 'string') return mlResponse.body; + try { + return JSON.parse(mlResponse.body); + } catch { + return { rawBody: mlResponse.body }; + } + })(),src/plugins/chat/public/components/chat_window.tsx (1)
87-96: Direct property mutation on chatService using type assertion.The pattern
(chatService as any).availableTools = state.toolDefinitionsbypasses TypeScript's type system. Consider whetheravailableToolsshould be part of the public ChatService interface, or if there's a setter method that should be used instead.src/core/public/chat/types.ts (3)
71-77: Consider extendingBaseMessagefor consistency.
ToolMessagedoesn't extendBaseMessage, which breaks the pattern. WhileToolMessagehastoolCallIdinstead ofname, it could still extendBaseMessagewithrole: 'tool'override.-export interface ToolMessage { +export interface ToolMessage extends BaseMessage { id: string; content: string; role: 'tool'; toolCallId: string; error?: string; }
135-143: Consider using a more specific type instead ofanyfor observable.The
sendMessageandsendMessageWithWindowmethods returnobservable: any. Using a typed observable (e.g.,Observable<BaseEvent>) would improve type safety and IDE support.+import { BaseEvent } from './ag_ui_agent'; // or appropriate import path + sendMessage( content: string, messages: Message[] - ): Promise<{ observable: any; userMessage: UserMessage }>; + ): Promise<{ observable: Observable<BaseEvent>; userMessage: UserMessage }>; sendMessageWithWindow( content: string, messages: Message[], options?: { clearConversation?: boolean } - ): Promise<{ observable: any; userMessage: UserMessage }>; + ): Promise<{ observable: Observable<BaseEvent>; userMessage: UserMessage }>;
181-189: Use specific types instead ofanyfor provider registration.The
suggestedActionsServiceusesanyfor the provider parameter. Consider defining a properSuggestedActionsProviderinterface to enforce type safety at plugin boundaries.+export interface SuggestedActionsProvider { + // Define the provider interface based on actual requirements +} + setSuggestedActionsService(service: { registerProvider(provider: any): void }): void; +// Consider: +// setSuggestedActionsService(service: { registerProvider(provider: SuggestedActionsProvider): void }): void;src/plugins/chat/public/services/chat_service.test.ts (1)
87-98: MockopenWindow/closeWindowshould update state for consistency.The mock implementations trigger callbacks but don't update
isWindowOpenstate. This could cause tests to pass even when real implementation behavior differs.openWindow: jest.fn(() => { const currentState = mockWindowState$.getValue(); if (!currentState.isWindowOpen) { + mockWindowState$.next({ ...currentState, isWindowOpen: true }); mockWindowOpenCallbacks.forEach((callback) => callback()); } }), closeWindow: jest.fn(() => { const currentState = mockWindowState$.getValue(); if (currentState.isWindowOpen) { + mockWindowState$.next({ ...currentState, isWindowOpen: false }); mockWindowCloseCallbacks.forEach((callback) => callback()); } }),src/plugins/chat/public/services/chat_service.ts (1)
77-89: Consider graceful degradation instead of throwing errors.Multiple methods throw
Error('Core chat service not available')whencoreChatServiceis undefined. Since these are public methods that could be called early in the lifecycle, consider returning sensible defaults or using optional chaining where appropriate.For read-only accessors, a fallback might be safer:
public getThreadId = () => { if (!this.coreChatService) { - throw new Error('Core chat service not available'); + console.warn('Core chat service not available, returning empty thread ID'); + return ''; } return this.coreChatService.getThreadId(); };Alternatively, document the precondition that core service must be available, or make the dependency required in the constructor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (54)
changelogs/fragments/10983.yml(1 hunks)changelogs/fragments/10999.yml(1 hunks)src/core/public/chat/chat_service.mock.ts(1 hunks)src/core/public/chat/chat_service.test.ts(1 hunks)src/core/public/chat/chat_service.ts(1 hunks)src/core/public/chat/index.ts(1 hunks)src/core/public/chat/types.ts(1 hunks)src/core/public/core_system.ts(9 hunks)src/core/public/index.ts(4 hunks)src/core/public/mocks.ts(3 hunks)src/core/public/plugins/plugin_context.ts(2 hunks)src/core/public/plugins/plugins_service.test.ts(3 hunks)src/plugins/chat/common/chat_capabilities.test.ts(1 hunks)src/plugins/chat/common/chat_capabilities.ts(1 hunks)src/plugins/chat/common/types.ts(2 hunks)src/plugins/chat/opensearch_dashboards.json(1 hunks)src/plugins/chat/public/actions/graph_timeseries_data_action.test.tsx(1 hunks)src/plugins/chat/public/actions/graph_timeseries_data_action.tsx(2 hunks)src/plugins/chat/public/components/chat_header_button.test.tsx(14 hunks)src/plugins/chat/public/components/chat_header_button.tsx(5 hunks)src/plugins/chat/public/components/chat_window.tsx(5 hunks)src/plugins/chat/public/components/graph_visualization/error_boundary.tsx(0 hunks)src/plugins/chat/public/components/graph_visualization/error_display.tsx(0 hunks)src/plugins/chat/public/components/graph_visualization/error_handler.ts(0 hunks)src/plugins/chat/public/components/graph_visualization/validation.ts(0 hunks)src/plugins/chat/public/plugin.test.ts(4 hunks)src/plugins/chat/public/plugin.ts(6 hunks)src/plugins/chat/public/services/ag_ui_agent.ts(2 hunks)src/plugins/chat/public/services/chat_context_manager.test.ts(0 hunks)src/plugins/chat/public/services/chat_context_manager.ts(0 hunks)src/plugins/chat/public/services/chat_event_handler.test.ts(1 hunks)src/plugins/chat/public/services/chat_event_handler.ts(1 hunks)src/plugins/chat/public/services/chat_service.test.ts(16 hunks)src/plugins/chat/public/services/chat_service.ts(15 hunks)src/plugins/chat/public/types.ts(2 hunks)src/plugins/chat/server/index.ts(1 hunks)src/plugins/chat/server/plugin.ts(1 hunks)src/plugins/chat/server/routes/index.test.ts(4 hunks)src/plugins/chat/server/routes/index.ts(3 hunks)src/plugins/chat/server/routes/ml_routes/generic_ml_router.ts(1 hunks)src/plugins/chat/server/routes/ml_routes/ml_agent_router.ts(1 hunks)src/plugins/chat/server/routes/ml_routes/ml_commons_agent.test.ts(0 hunks)src/plugins/chat/server/routes/ml_routes/ml_commons_agent.ts(0 hunks)src/plugins/chat/server/routes/ml_routes/router_registry.ts(1 hunks)src/plugins/data_source_management/public/index.ts(1 hunks)src/plugins/explore/opensearch_dashboards.json(1 hunks)src/plugins/explore/public/actions/ask_ai_action.test.ts(3 hunks)src/plugins/explore/public/actions/ask_ai_action.ts(1 hunks)src/plugins/explore/public/build_services.ts(1 hunks)src/plugins/explore/public/components/ask_ai_action_item/ask_ai_action_item.test.tsx(3 hunks)src/plugins/explore/public/components/ask_ai_action_item/ask_ai_action_item.tsx(3 hunks)src/plugins/explore/public/components/tabs/action_bar/action_bar.tsx(2 hunks)src/plugins/explore/public/plugin.ts(1 hunks)src/plugins/explore/public/types.ts(1 hunks)
💤 Files with no reviewable changes (8)
- src/plugins/chat/server/routes/ml_routes/ml_commons_agent.ts
- src/plugins/chat/public/components/graph_visualization/validation.ts
- src/plugins/chat/server/routes/ml_routes/ml_commons_agent.test.ts
- src/plugins/chat/public/components/graph_visualization/error_display.tsx
- src/plugins/chat/public/services/chat_context_manager.ts
- src/plugins/chat/public/services/chat_context_manager.test.ts
- src/plugins/chat/public/components/graph_visualization/error_boundary.tsx
- src/plugins/chat/public/components/graph_visualization/error_handler.ts
🧰 Additional context used
🧬 Code graph analysis (28)
src/plugins/chat/public/services/chat_event_handler.test.ts (1)
src/core/public/chat/types.ts (1)
ToolMessage(71-77)
src/plugins/explore/public/types.ts (1)
src/plugins/explore/public/services/slot_registry/slot_registry_service.ts (1)
SlotRegistryService(65-116)
src/plugins/chat/public/services/ag_ui_agent.ts (1)
src/plugins/chat/common/types.ts (1)
RunAgentInput(111-111)
src/plugins/chat/server/routes/ml_routes/router_registry.ts (4)
src/core/public/index.ts (1)
Capabilities(361-361)src/plugins/chat/common/chat_capabilities.ts (1)
hasInvestigationCapabilities(26-28)src/plugins/chat/server/routes/ml_routes/ml_agent_router.ts (1)
MLAgentRouterFactory(49-74)src/plugins/chat/server/routes/ml_routes/generic_ml_router.ts (1)
GenericMLRouter(79-176)
src/plugins/explore/public/actions/ask_ai_action.test.ts (2)
src/core/public/chat/types.ts (1)
ChatServiceStart(195-203)src/plugins/explore/public/actions/ask_ai_action.ts (1)
createAskAiAction(13-29)
src/plugins/chat/public/plugin.test.ts (2)
src/core/public/chat/chat_service.ts (1)
ChatService(19-169)src/plugins/chat/public/plugin.ts (1)
ChatPlugin(41-193)
src/plugins/chat/public/types.ts (1)
src/plugins/data_source_management/public/index.ts (1)
DataSourceManagementPluginSetup(18-18)
src/plugins/chat/server/routes/ml_routes/ml_agent_router.ts (1)
src/core/server/index.ts (4)
RequestHandlerContext(436-460)OpenSearchDashboardsRequest(187-187)OpenSearchDashboardsResponseFactory(215-215)IOpenSearchDashboardsResponse(191-191)
src/core/public/index.ts (1)
src/core/public/chat/types.ts (2)
ChatServiceSetup(170-190)ChatServiceStart(195-203)
src/core/public/plugins/plugins_service.test.ts (3)
src/core/public/chat/chat_service.mock.ts (1)
coreChatServiceMock(50-53)src/core/public/chat/index.ts (1)
coreChatServiceMock(24-24)src/core/public/mocks.ts (1)
coreChatServiceMock(68-68)
src/plugins/chat/server/routes/ml_routes/generic_ml_router.ts (2)
src/core/server/index.ts (4)
RequestHandlerContext(436-460)OpenSearchDashboardsRequest(187-187)OpenSearchDashboardsResponseFactory(215-215)IOpenSearchDashboardsResponse(191-191)src/plugins/chat/server/routes/ml_routes/ml_agent_router.ts (1)
MLAgentRouter(19-43)
src/core/public/chat/chat_service.test.ts (2)
src/core/public/chat/chat_service.ts (1)
ChatService(19-169)src/core/public/chat/types.ts (1)
ChatImplementationFunctions(149-165)
src/core/public/chat/chat_service.mock.ts (4)
src/core/public/chat/types.ts (2)
ChatServiceSetup(170-190)ChatServiceStart(195-203)src/core/public/chat/index.ts (3)
ChatServiceSetup(8-8)ChatServiceStart(9-9)coreChatServiceMock(24-24)src/core/public/index.ts (2)
ChatServiceSetup(439-439)ChatServiceStart(440-440)src/core/public/mocks.ts (1)
coreChatServiceMock(68-68)
src/plugins/chat/public/plugin.ts (3)
src/core/public/index.ts (2)
CoreSetup(262-298)CoreStart(321-358)src/core/public/chat/chat_service.ts (1)
ChatService(19-169)src/core/public/chat/index.ts (1)
ChatService(23-23)
src/plugins/explore/public/components/tabs/action_bar/action_bar.tsx (1)
src/plugins/opensearch_dashboards_utils/public/index.ts (1)
of(41-41)
src/plugins/explore/public/components/ask_ai_action_item/ask_ai_action_item.tsx (4)
src/plugins/explore/public/types/log_actions.ts (1)
LogActionItemProps(37-46)src/core/public/chat/types.ts (1)
ChatServiceStart(195-203)src/core/public/index.ts (1)
ChatServiceStart(440-440)src/plugins/context_provider/public/hooks/use_dynamic_context.ts (1)
useDynamicContext(18-78)
src/plugins/chat/server/routes/index.test.ts (1)
src/plugins/chat/server/routes/index.ts (1)
defineRoutes(81-161)
src/core/public/chat/chat_service.ts (2)
src/core/public/chat/types.ts (5)
ChatServiceSetup(170-190)ChatServiceStart(195-203)ChatImplementationFunctions(149-165)ChatWindowState(97-101)Message(82-87)src/core/public/index.ts (5)
ChatServiceSetup(439-439)ChatServiceStart(440-440)ChatImplementationFunctions(441-441)ChatWindowState(443-443)Message(442-442)
src/plugins/chat/common/chat_capabilities.ts (1)
src/plugins/chat/server/index.ts (1)
config(13-19)
src/plugins/chat/public/actions/graph_timeseries_data_action.test.tsx (1)
src/plugins/chat/public/actions/graph_timeseries_data_action.tsx (1)
useGraphTimeseriesDataAction(42-250)
src/plugins/chat/common/chat_capabilities.test.ts (2)
src/plugins/chat/common/chat_capabilities.ts (4)
hasInvestigationCapabilities(26-28)ContextProviderConfig(17-19)ChatConfig(9-12)isChatEnabled(42-59)src/plugins/chat/server/index.ts (1)
config(13-19)
src/plugins/chat/public/components/chat_header_button.test.tsx (2)
src/core/public/mocks.ts (1)
coreMock(206-213)src/plugins/chat/public/components/chat_header_button.tsx (1)
ChatHeaderButton(39-251)
src/plugins/explore/public/build_services.ts (2)
src/plugins/explore/public/services/slot_registry/index.ts (1)
SlotRegistryService(7-7)src/plugins/explore/public/services/slot_registry/slot_registry_service.ts (1)
SlotRegistryService(65-116)
src/plugins/explore/public/plugin.ts (2)
src/plugins/explore/public/actions/ask_ai_action.ts (1)
createAskAiAction(13-29)src/plugins/explore/public/services/log_action_registry.ts (1)
logActionRegistry(76-76)
src/plugins/explore/public/components/ask_ai_action_item/ask_ai_action_item.test.tsx (3)
src/core/public/chat/types.ts (1)
ChatServiceStart(195-203)src/core/public/chat/index.ts (1)
ChatServiceStart(9-9)src/core/public/index.ts (1)
ChatServiceStart(440-440)
src/plugins/chat/public/components/chat_window.tsx (1)
src/core/public/chat/types.ts (1)
Message(82-87)
src/plugins/chat/server/routes/index.ts (2)
src/plugins/chat/server/routes/ml_routes/router_registry.ts (1)
MLAgentRouterRegistry(16-50)src/plugins/chat/server/routes/ml_routes/ml_agent_router.ts (1)
MLAgentRouterFactory(49-74)
src/plugins/chat/public/services/chat_service.test.ts (4)
src/core/public/chat/chat_service.ts (1)
ChatService(19-169)src/plugins/chat/public/services/ag_ui_agent.ts (1)
AgUiAgent(22-142)src/core/public/chat/types.ts (1)
ChatServiceStart(195-203)src/core/public/index.ts (1)
ChatServiceStart(440-440)
🪛 YAMLlint (1.37.1)
changelogs/fragments/10983.yml
[error] 2-2: syntax error: expected , but found ''
(syntax)
changelogs/fragments/10999.yml
[error] 2-2: syntax error: expected , but found '['
(syntax)
⏰ 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). (65)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: bundle-analyzer
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: lighthouse
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup3)
| refactor: | ||
| - [Chat] Refactor chat architecture: Extract shared infrastructure into a core chat service ([#10983](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10983)) No newline at end of file |
There was a problem hiding this comment.
Quote the list item to fix YAML syntax error
Same issue here: the list value starts with [, which is not allowed for an unquoted scalar.
refactor:
-- [Chat] Refactor chat architecture: Extract shared infrastructure into a core chat service ([#10983](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10983))
+- "[Chat] Refactor chat architecture: Extract shared infrastructure into a core chat service ([#10983](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10983))"📝 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.
| refactor: | |
| - [Chat] Refactor chat architecture: Extract shared infrastructure into a core chat service ([#10983](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10983)) | |
| refactor: | |
| - "[Chat] Refactor chat architecture: Extract shared infrastructure into a core chat service ([#10983](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10983))" |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 2-2: syntax error: expected , but found ''
(syntax)
🤖 Prompt for AI Agents
In changelogs/fragments/10983.yml around lines 1 to 2, the list item begins with
'[' which breaks YAML parsing; wrap the entire list string in quotes (e.g. '"'
or "'") so the scalar is quoted, preserving the content exactly (including the
bracketed PR link) to fix the YAML syntax error.
| fix: | ||
| - [BUG][Chat] Unregister tools when component unmount ([#10999](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10999)) No newline at end of file |
There was a problem hiding this comment.
Quote the list item to fix YAML syntax error
YAML plain scalars cannot start with [; this fragment will fail parsing. Wrap the string in quotes.
fix:
-- [BUG][Chat] Unregister tools when component unmount ([#10999](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10999))
+- "[BUG][Chat] Unregister tools when component unmount ([#10999](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10999))"📝 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.
| fix: | |
| - [BUG][Chat] Unregister tools when component unmount ([#10999](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10999)) | |
| fix: | |
| - "[BUG][Chat] Unregister tools when component unmount ([#10999](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/10999))" |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 2-2: syntax error: expected , but found '['
(syntax)
🤖 Prompt for AI Agents
In changelogs/fragments/10999.yml around lines 1 to 2, the YAML fragment uses a
plain scalar that starts with '[' which is invalid; wrap the entire list item
string in quotes (single or double) so the line becomes a quoted string value
under fix:, preserving the same text content and indentation to ensure the
fragment parses as valid YAML.
| let cachedClientKey: string | null = null; | ||
| let detectionAttempted = false; |
There was a problem hiding this comment.
Module-level mutable state causes cross-request contamination.
cachedClientKey and detectionAttempted are module-level variables shared across all incoming requests. In a Node.js server, this module is loaded once, so:
- If the first request has a certain context structure, all subsequent requests assume the same structure
- In multi-cluster or multi-tenant scenarios, different requests may have different context providers
- The cache is never invalidated, causing stale results if ML client availability changes
Move the caching to be request-scoped or remove it entirely:
-let cachedClientKey: string | null = null;
-let detectionAttempted = false;
-
-function findMLClient(context: RequestHandlerContext) {
- // Use cached result if available
- if (detectionAttempted) {
- if (cachedClientKey) {
- const contextItem = (context as any)[cachedClientKey];
- return contextItem?.client;
- }
- return undefined;
- }
-
- // First-time detection - cache the result
- for (const key of Object.keys(context)) {
- const contextItem = (context as any)[key];
- if (contextItem?.client?.request && typeof contextItem.client.request === 'function') {
- cachedClientKey = key; // Cache the key name
- detectionAttempted = true;
- return contextItem.client;
- }
- }
-
- detectionAttempted = true; // Mark as attempted even if not found
- return undefined;
-}
+function findMLClient(context: RequestHandlerContext) {
+ for (const key of Object.keys(context)) {
+ const contextItem = (context as any)[key];
+ if (contextItem?.client?.request && typeof contextItem.client.request === 'function') {
+ return contextItem.client;
+ }
+ }
+ return undefined;
+}📝 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.
| let cachedClientKey: string | null = null; | |
| let detectionAttempted = false; | |
| function findMLClient(context: RequestHandlerContext) { | |
| for (const key of Object.keys(context)) { | |
| const contextItem = (context as any)[key]; | |
| if (contextItem?.client?.request && typeof contextItem.client.request === 'function') { | |
| return contextItem.client; | |
| } | |
| } | |
| return undefined; | |
| } |
🤖 Prompt for AI Agents
In src/plugins/chat/server/routes/ml_routes/generic_ml_router.ts around lines
20-21, the module-level variables `cachedClientKey` and `detectionAttempted`
create cross-request state; move the cache to request scope or remove it: stop
using module-level mutable variables and instead compute/detect the ML client
per request (or attach a cache to the request object or per-tenant context) and
ensure any cache has proper invalidation/TTL tied to the request/tenant
lifecycle; update all call sites to read/write the per-request/tenant cache and
remove the global variables so no stale or shared state persists across requests
or clusters.
| // Register context with cleanup on unmount (true = cleanup when component unmounts) | ||
| useDynamicContext(contextData, false); |
There was a problem hiding this comment.
Comment contradicts the actual behavior—context will NOT be cleaned up on unmount.
The comment states "cleanup on unmount" but false is passed as the second parameter. According to useDynamicContext signature (from src/plugins/context_provider/public/hooks/use_dynamic_context.ts), shouldCleanupOnUnmount = true enables cleanup on unmount. Passing false explicitly disables cleanup.
This contradicts the PR objective to "Unregister tools when component unmount". Either:
- Change
falsetotrueto enable cleanup, or - Update the comment to reflect the intentional no-cleanup behavior
If cleanup is desired per the PR objective:
- // Register context with cleanup on unmount (true = cleanup when component unmounts)
- useDynamicContext(contextData, false);
+ // Register context with cleanup on unmount
+ useDynamicContext(contextData, true);📝 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.
| // Register context with cleanup on unmount (true = cleanup when component unmounts) | |
| useDynamicContext(contextData, false); | |
| // Register context with cleanup on unmount | |
| useDynamicContext(contextData, true); |
🤖 Prompt for AI Agents
In
src/plugins/explore/public/components/ask_ai_action_item/ask_ai_action_item.tsx
around lines 67-68 the comment says "cleanup on unmount" but the call uses
useDynamicContext(contextData, false) which disables cleanup; to satisfy the PR
objective of unregistering tools on unmount, change the second argument from
false to true so the hook will perform cleanup on component unmount (or if the
no-cleanup behavior was intentional, update the comment to reflect that cleanup
is disabled).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10999 +/- ##
==========================================
- Coverage 60.77% 60.77% -0.01%
==========================================
Files 4537 4537
Lines 122381 122388 +7
Branches 20535 20536 +1
==========================================
- Hits 74381 74380 -1
- Misses 42743 42751 +8
Partials 5257 5257
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh
force-pushed
the
chat-fix-unregister-tool
branch
from
b301383 to
9ffe6e9
Compare
ananzh
added
ai
3 participants
Description
Note: Need #10983 to merge first
This PR allows to tools to be unregistered when component unmount. So when navigate from one page to another, the page tool will be unregistered and can't be included to tools to AG-UI.
Issues Resolved
NA
Screenshot
Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.