feat: add context awareness for explore visualizations#11134
feat: add context awareness for explore visualizations#11134wanglam merged 2 commits intoopensearch-project:mainfrom
Conversation
SuZhou-Joe
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
Maosaic,
TackAdam,
ZilongX,
abbyhu2000,
ananzh,
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
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds an "Ask AI" embeddable action for Explore visualizations that captures images via html2canvas, posts visualization data to a summarization endpoint, and injects context into the assistant store to open a chat. Also extends chat/message types and rendering to support multimodal (text + binary) content. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExploreUI as Explore UI
participant AskAIAction as AskAI Action
participant Html2Canvas as html2canvas
participant API as /api/visualizations/summary
participant ContextStore as Assistant Context Store
participant Chat as Chat Service
participant ChatWindow as Chat Window
User->>ExploreUI: Click "Ask AI" in context menu
ExploreUI->>AskAIAction: execute()
AskAIAction->>Html2Canvas: capture visualization
Html2Canvas-->>AskAIAction: image (base64 JPEG)
AskAIAction->>API: POST image + metadata
API-->>AskAIAction: response
AskAIAction->>ContextStore: add visualization context
ContextStore-->>AskAIAction: confirm
AskAIAction->>Chat: sendMessage(content + messages)
Chat->>ChatWindow: open & render message
ChatWindow-->>User: display chat + image
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SuZhou-Joe
force-pushed
the
feature/visualization_summary
branch
from
3535b9c to
f9e1a26
Compare
![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
🤖 Fix all issues with AI Agents
In @src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx:
- Around line 63-71: The grouping array stores unbound method references
getDisplayName and getIconType which can lose their class this context; fix by
ensuring these are bound: either convert getDisplayName and getIconType to arrow
function class properties so they retain this, or explicitly bind them in the
class constructor (e.g., this.getDisplayName = this.getDisplayName.bind(this))
and keep grouping as is; update references in the class containing grouping and
ASK_AI_EMBEDDABLE_ACTION accordingly so calls always have the correct this.
- Around line 147-148: The catch block in ask_ai_embeddable_action.tsx currently
swallows errors ("catch (captureError) { // do nothing }"); update the catch to
surface the failure by logging the error (e.g., use the available logger or
console.error) and/or set user-visible feedback (e.g., call the UI error
notifier or set an error state) so image capture/API failures are recorded and
the user is informed; modify the catch surrounding the image capture/API call in
the function handling the capture (look for the catch with the captureError
identifier) to include a descriptive log message and an appropriate user-facing
error notification.
In @src/plugins/explore/public/plugin.test.ts:
- Around line 247-270: The test in the "setup" spec calls
plugin.setup(coreSetup, setupDeps) and asserts coreSetup.application.register
was calledTimes(4) but only verifies three registrations (ids 'explore/logs',
'explore/traces', 'explore/metrics'); either add a fourth
expect(coreSetup.application.register).toHaveBeenCalledWith(expect.objectContaining({
id: 'explore', title: 'Explore' })) to verify the main explore app registration,
or change the toHaveBeenCalledTimes(4) to toHaveBeenCalledTimes(3) if the main
'explore' registration was intentionally removed; update the test around
coreSetup.application.register accordingly.
🧹 Nitpick comments (7)
src/plugins/visualizations/server/routes/visualization_summary.test.ts (2)
12-12: Remove unused import.The
ofimport fromrxjsis not used anywhere in this test file.🔎 Proposed fix
-import { of } from 'rxjs'; import { registerVisualizationSummaryRoute } from './visualization_summary';
263-316: Consider adding assertions for the transport request calls when using data source.The test verifies
getClientis called with the correctdataSourceId, but it doesn't assert that subsequent ML config and agent execute calls are made through the data source client's transport rather than the default one.🔎 Proposed enhancement
await routeHandler(mockContextWithDataSource, mockRequestWithDataSource, mockResponse); expect(mockContextWithDataSource.dataSource.opensearch.getClient).toHaveBeenCalledWith( 'ds-123' ); + + // Verify that the data source client's transport was used for requests + const dsClient = await mockContextWithDataSource.dataSource.opensearch.getClient('ds-123'); + expect(dsClient.transport.request).toHaveBeenCalledWith( + expect.objectContaining({ + method: 'GET', + path: '/_plugins/_ml/config/os_visualization_summary', + }) + ); });src/plugins/explore/public/actions/ask_ai_embeddable_action.test.tsx (2)
7-7: Remove unused imports.
renderandscreenfrom@testing-library/reactare imported but never used in the tests.🔎 Proposed fix
-import { render, screen, waitFor } from '@testing-library/react'; +import { waitFor } from '@testing-library/react';
197-206: Consider adding assertion for toast notification on error.The test verifies that context is still added when the API fails, but it doesn't verify that an error toast is shown to the user. Based on the implementation, errors in the outer try-catch show a danger toast.
Note: The implementation swallows errors in the inner try-catch (capture/API call) silently, so the toast may not be shown in this case. Consider whether this behavior is intentional.
src/plugins/visualizations/server/routes/visualization_summary.ts (1)
63-67: Consider adding a maximum length validation for the visualization payload.Base64-encoded images can be very large. Consider adding a
maxLengthconstraint to prevent memory issues or abuse.🔎 Proposed enhancement
body: schema.object({ visualization: schema.string({ minLength: 1, + // 10MB base64 ≈ 13.3M characters + maxLength: 15_000_000, }), }),src/plugins/explore/public/plugin.test.ts (1)
6-6: Remove unused import.
Reactis imported but not used in this test file.🔎 Proposed fix
import { ExplorePlugin } from './plugin'; -import React from 'react';Note: This line should be removed entirely since React is not imported on line 6 based on the code provided. If React is used elsewhere (e.g., in JSX), keep it.
src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx (1)
113-120: Overlay cleanup logic is duplicated.The overlay removal code appears in both the
finallyblock (lines 151-155) and the outercatchblock (lines 193-196). However, since the innertry-catch-finallyhandles its own cleanup infinally, the outer catch cleanup handles errors from context/chat operations. This is correct but could be clearer.Consider extracting the cleanup logic into a helper function for clarity and to ensure consistent cleanup:
🔎 Proposed refactor
+ // Helper to clean up overlay + const removeOverlay = () => { + if (overlayContainer && embeddableContainer) { + ReactDOM.unmountComponentAtNode(overlayContainer); + embeddableContainer.removeChild(overlayContainer); + } + }; + try { // ... capture logic ... } finally { - // Remove loading overlay - if (overlayContainer && embeddableContainer) { - ReactDOM.unmountComponentAtNode(overlayContainer); - embeddableContainer.removeChild(overlayContainer); - } + removeOverlay(); } // ... context/chat logic ... } catch (error) { - // Remove loading overlay on error - if (overlayContainer && embeddableContainer) { - ReactDOM.unmountComponentAtNode(overlayContainer); - embeddableContainer.removeChild(overlayContainer); - } + removeOverlay(); // ... error handling ... }Also applies to: 149-155, 191-196
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
changelogs/fragments/11134.ymlpackage.jsonsrc/core/public/mocks.tssrc/plugins/explore/opensearch_dashboards.jsonsrc/plugins/explore/public/actions/ask_ai_embeddable_action.scsssrc/plugins/explore/public/actions/ask_ai_embeddable_action.test.tsxsrc/plugins/explore/public/actions/ask_ai_embeddable_action.tsxsrc/plugins/explore/public/plugin.test.tssrc/plugins/explore/public/plugin.tssrc/plugins/explore/public/types.tssrc/plugins/visualizations/opensearch_dashboards.jsonsrc/plugins/visualizations/server/plugin.tssrc/plugins/visualizations/server/routes/index.tssrc/plugins/visualizations/server/routes/visualization_summary.test.tssrc/plugins/visualizations/server/routes/visualization_summary.ts
🧰 Additional context used
🧬 Code graph analysis (9)
src/core/public/mocks.ts (2)
src/core/public/chat/chat_service.mock.ts (1)
coreChatServiceMock(50-53)src/core/public/chat/index.ts (1)
coreChatServiceMock(24-24)
src/plugins/visualizations/server/routes/index.ts (2)
src/core/server/index.ts (1)
IRouter(217-217)src/plugins/visualizations/server/routes/visualization_summary.ts (1)
registerVisualizationSummaryRoute(58-154)
src/plugins/visualizations/server/plugin.ts (1)
src/plugins/visualizations/server/routes/index.ts (1)
setupRoutes(15-17)
src/plugins/explore/public/actions/ask_ai_embeddable_action.test.tsx (1)
src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx (1)
AskAIEmbeddableAction(58-206)
src/plugins/visualizations/server/routes/visualization_summary.ts (1)
src/core/server/index.ts (5)
RequestHandlerContext(436-460)OpenSearchClient(146-146)OpenSearchDashboardsRequest(187-187)IRouter(217-217)IOpenSearchDashboardsResponse(191-191)
src/plugins/explore/public/plugin.ts (1)
src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx (1)
AskAIEmbeddableAction(58-206)
src/plugins/explore/public/types.ts (3)
src/plugins/chat/server/index.ts (1)
ChatPluginStart(28-28)src/plugins/chat/public/types.ts (1)
ChatPluginStart(16-18)src/plugins/chat/public/index.ts (1)
ChatPluginStart(18-18)
src/plugins/visualizations/server/routes/visualization_summary.test.ts (1)
src/plugins/visualizations/server/routes/visualization_summary.ts (1)
registerVisualizationSummaryRoute(58-154)
src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx (2)
src/plugins/context_provider/public/types.ts (1)
ContextProviderStart(38-45)src/plugins/ui_actions/public/index.ts (1)
IncompatibleActionError(44-44)
⏰ 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). (68)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Lint and validate
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: linkchecker
- GitHub Check: lighthouse
- GitHub Check: bundle-analyzer
🔇 Additional comments (25)
src/plugins/explore/public/actions/ask_ai_embeddable_action.scss (1)
1-9: LGTM!The loading overlay styling is well-structured, uses appropriate theme variables, and follows EUI conventions for overlay positioning and z-index management.
src/plugins/visualizations/opensearch_dashboards.json (1)
7-7: LGTM!Adding
dataSourceas an optional plugin is appropriate and maintains backward compatibility since it's optional rather than required.src/core/public/mocks.ts (3)
52-52: LGTM!The import and export of
coreChatServiceMockfollow the established pattern for other service mocks in this file.Also applies to: 68-68
97-97: LGTM!The chat service is correctly integrated into the setup mock, consistent with other core services.
121-121: LGTM!The chat service is correctly integrated into the start mock, following the same pattern as other core services.
changelogs/fragments/11134.yml (1)
1-3: LGTM!The changelog entries clearly document the new features and properly reference the PR.
package.json (1)
269-269: Version 1.4.1 is current and has no known security vulnerabilities.Verification confirms that html2canvas 1.4.1 is the latest official release and no known CVE-style vulnerabilities exist in this version according to Snyk and ReversingLabs security scanners.
src/plugins/explore/opensearch_dashboards.json (1)
24-24: LGTM! Correctly declares chat as an optional dependency.The addition of "chat" to optionalPlugins properly enables the optional integration with the chat plugin for the "Ask AI" feature, while maintaining backward compatibility when the chat plugin is not available.
src/plugins/visualizations/server/plugin.ts (1)
103-105: LGTM! Routing setup properly integrated into plugin lifecycle.The HTTP router is correctly created and passed to
setupRoutes, which registers the new/api/visualizations/summaryendpoint. The placement after usage collector registration is appropriate.src/plugins/explore/public/types.ts (2)
41-41: LGTM! Import follows established conventions.The import of
ChatPluginStartis correctly placed alongside other plugin type imports and follows the established pattern for optional plugin dependencies.
127-127: LGTM! Optional dependency properly typed.The addition of
chat?: ChatPluginStartcorrectly declares the chat plugin as an optional dependency in the start lifecycle, consistent with the manifest declaration and enabling conditional feature integration.src/plugins/visualizations/server/routes/index.ts (1)
15-17: LGTM! Clean routing entry point with good extensibility.The
setupRoutesfunction provides a clean delegation pattern for route registration. This design makes it easy to add additional routes in the future by importing and calling additional registration functions within this module.src/plugins/explore/public/plugin.ts (2)
78-79: LGTM! Imports correctly added for the new action integration.The imports for
AskAIEmbeddableActionandCONTEXT_MENU_TRIGGERare appropriately placed and enable the conditional registration of the "Ask AI" context menu action.
519-527: LGTM! Conditional action registration properly implemented.The action is correctly registered only when both
chatandcontextProviderplugins are available, ensuring graceful degradation when optional dependencies are missing. The action is instantiated with the required dependencies and properly attached to theCONTEXT_MENU_TRIGGER. Action cleanup is handled automatically by the uiActions framework when the service stops (viaservice.clear()), so no manual unregistration is needed.src/plugins/visualizations/server/routes/visualization_summary.test.ts (3)
15-113: Good test coverage for the happy path and route registration.The test setup properly mocks the router, context, and response objects. The tests verify route registration at the correct path and validate that the ML config API is called with the expected parameters.
115-129: Good edge case coverage for missing agent ID.This test validates the 404 response when the agent ID is not present in the configuration.
221-261: Good error handling test coverage.Tests properly verify that config API and predict API errors are handled with appropriate error messages and status codes.
src/plugins/explore/public/actions/ask_ai_embeddable_action.test.tsx (2)
101-133: Good compatibility test coverage.Tests properly verify that
isCompatiblereturns the expected values for different embeddable types and context provider availability.
157-171: Verify the expected base64 payload format.The test expects
mockImageDatabut the mock returnsdata:image/png;base64,mockImageData. The implementation splits on,and takes[1], so this should work. However, the mock returns PNG format while the implementation uses JPEG at 0.5 quality.Consider aligning the mock format with the actual implementation format for consistency:
🔎 Proposed fix
// Mock html2canvas (html2canvas as jest.Mock).mockResolvedValue({ - toDataURL: jest.fn().mockReturnValue('data:image/png;base64,mockImageData'), + toDataURL: jest.fn().mockReturnValue('data:image/jpeg;base64,mockImageData'), });src/plugins/visualizations/server/routes/visualization_summary.ts (2)
21-22: LGTM - Constants are appropriately defined.The ML Commons API prefix and config name are clearly defined as constants for maintainability.
27-46: Good data source abstraction.The helper function cleanly handles both data-source-specific and default transport clients.
src/plugins/explore/public/plugin.test.ts (2)
319-332: Good integration test for Ask AI action registration.The test properly verifies that when both
chatandcontextProviderare available, theAskAIEmbeddableActionis created with the correct dependencies and registered with the UI actions system.
334-362: Good negative test cases for conditional registration.Tests properly verify that the action is not registered when either
chatorcontextProvideris unavailable, ensuring the feature degrades gracefully.src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx (2)
27-47: Loading overlay component is well-structured.Good use of EUI components for consistent styling and i18n for localization.
169-180: Good context structure for AI assistant integration.The visualization context includes relevant metadata (title, type, time range, query, filters, index, summary) that provides comprehensive information for the AI assistant.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11134 +/- ##
==========================================
+ Coverage 60.20% 60.25% +0.05%
==========================================
Files 4628 4629 +1
Lines 127936 128017 +81
Branches 21570 21594 +24
==========================================
+ Hits 77018 77139 +121
+ Misses 45447 45396 -51
- Partials 5471 5482 +11
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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/plugins/chat/common/types.ts (1)
50-55: Schema mismatch:BaseMessageSchema.contentdoesn't match the TypeScript type.The TypeScript
BaseMessage.contentinsrc/core/public/chat/types.ts(line 47) isstring | InputContent[], butBaseMessageSchema.contenthere isz.string().optional(). This could cause validation failures for messages with array content when validated against the base schema.Consider updating for consistency:
Suggested fix
export const BaseMessageSchema = z.object({ id: z.string(), role: z.string(), - content: z.string().optional(), + content: z.union([z.string(), z.array(InputContentSchema)]).optional(), name: z.string().optional(), });src/plugins/chat/public/components/chat_window.tsx (1)
206-209: Fix type mismatch: Normalizemessage.contentbefore resending inhandleResendMessageIn
handleResendMessage(line 206),message.contentis passed directly tochatService.sendMessage(), which expects astringparameter. SinceUserMessage.contentcan bestring | InputContent[], resending multimodal messages with array content creates a type mismatch. Extract the text content or convert array content to a string representation before passing tosendMessage.
🤖 Fix all issues with AI agents
In @src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx:
- Around line 147-157: The inner finally unmounts and removes the loading
overlay (overlayContainer from embeddableContainer) which can be removed again
by the outer catch/finally; instead ensure the overlay is removed exactly once
by moving the unmount/remove logic out of the inner try/finally into the outer
finally (or add a boolean flag like overlayRemoved and check it before
unmounting), and keep references to overlayContainer and embeddableContainer
used in the removal; update code paths around capture logic,
contextStore.addContext, and chat.chatService.sendMessageWithWindow to rely on
the single removal point so duplicate removals cannot occur.
🧹 Nitpick comments (6)
packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (1)
476-495: Consider validating and normalizing the image format for Bedrock compatibility.The mimeType parsing could produce invalid formats for Bedrock. For example,
image/svg+xmlwould extractsvg+xml, which Bedrock doesn't support. AWS Bedrock only accepts specific formats:jpeg,png,gif, andwebp.Additionally, the
Bufferglobal is available in Node.js, but an explicit import would make the dependency clearer.♻️ Proposed fix with format validation
Add import at the top of the file:
import { Buffer } from 'buffer';Then update the format handling:
// Convert binary content to Bedrock image format // The AWS SDK expects image data as Uint8Array (bytes), not base64 string // Extract format from mimeType (e.g., 'image/jpeg' -> 'jpeg') - const format = block.mimeType?.includes('/') - ? block.mimeType.split('/')[1] - : block.mimeType || 'jpeg'; + const SUPPORTED_FORMATS = ['jpeg', 'jpg', 'png', 'gif', 'webp']; + let extractedFormat = block.mimeType?.includes('/') + ? block.mimeType.split('/')[1]?.split(';')[0] // Handle 'image/jpeg;charset=...' + : block.mimeType; + // Normalize 'jpg' to 'jpeg' for Bedrock + if (extractedFormat === 'jpg') extractedFormat = 'jpeg'; + const format = SUPPORTED_FORMATS.includes(extractedFormat) ? extractedFormat : 'jpeg';src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx (1)
115-120: Minor: Consider null-check before style mutation.The code modifies
embeddableContainer.style.positionwithout checking ifembeddableContainer.styleexists. While this is unlikely to fail in practice, defensive coding would be safer.src/core/public/chat/types.ts (1)
13-20: Consider requiring at least one data source field inBinaryInputContent.All data-providing fields (
id,url,data,filename) are optional. This could allow creating aBinaryInputContentwith onlymimeTypeset, which wouldn't be useful. Consider documenting or enforcing that at least one ofurlordatashould be present.src/plugins/chat/public/components/message_row.tsx (1)
44-44: Consider using a more specific type instead ofanyfor the block parameter.Using
anyloses type safety. Consider importing and using theInputContenttype or creating a local type for better type checking.Suggested improvement
+// Define block type for better type safety +type ContentBlock = { type: 'binary'; mimeType?: string; data?: string; filename?: string } + | { type: 'text'; text?: string } + | { text?: string }; if (Array.isArray(content)) { return ( <> - {content.map((block: any, index: number) => { + {content.map((block: ContentBlock, index: number) => {src/plugins/chat/public/services/chat_service.ts (2)
364-388: LGTM on multimodal message merging logic.The logic correctly handles the case where the last message is a user message with array content (multimodal), merging the new text content into the existing array. This preserves the image data while appending the text prompt.
One minor observation: the parameter
messagesis reassigned at line 373. While this works, creating a new variable would be clearer:Optional: Use a new variable for clarity
if (hasArrayContent && lastMessage) { - // Remove the last message from the array since we'll merge it with the new message - messages = messages.slice(0, -1); + // Create new array without the last message since we'll merge it with the new message + const previousMessages = messages.slice(0, -1); // Append text to the existing content array (preserves order from caller) userMessage = { ...lastMessage, id: this.generateMessageId(), content: [...(lastMessage.content as any[]), { type: 'text', text: content.trim() }], }; + + // Use previousMessages instead of messages below
376-380: Type assertionas any[]reduces type safety.Consider using the proper type from the core types or creating a type guard to ensure type safety:
Suggested improvement
+import type { InputContent } from '../../../core/public/chat'; userMessage = { ...lastMessage, id: this.generateMessageId(), - content: [...(lastMessage.content as any[]), { type: 'text', text: content.trim() }], + content: [...(lastMessage.content as InputContent[]), { type: 'text' as const, text: content.trim() }], };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/osd-agents/src/agents/langgraph/react_graph_nodes.tssrc/core/public/chat/types.tssrc/plugins/chat/common/types.tssrc/plugins/chat/public/components/chat_window.tsxsrc/plugins/chat/public/components/message_row.tsxsrc/plugins/chat/public/services/chat_service.tssrc/plugins/explore/public/actions/ask_ai_embeddable_action.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
src/plugins/chat/public/components/chat_window.tsx (3)
src/core/public/chat/types.ts (1)
Message(98-103)src/plugins/chat/common/types.ts (1)
Message(12-12)src/core/public/index.ts (1)
Message(442-442)
src/plugins/chat/public/components/message_row.tsx (2)
src/plugins/index_pattern_management/public/components/create_index_pattern_wizard/create_index_pattern_wizard.tsx (1)
renderContent(301-446)src/plugins/opensearch_dashboards_react/public/index.ts (1)
Markdown(45-45)
packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (1)
packages/osd-pm/dist/index.js (2)
block(44311-44311)Buffer(32620-32620)
src/plugins/chat/public/services/chat_service.ts (1)
src/core/public/chat/types.ts (1)
UserMessage(79-82)
src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx (2)
src/plugins/context_provider/public/types.ts (1)
ContextProviderStart(38-45)src/plugins/ui_actions/public/index.ts (1)
IncompatibleActionError(44-44)
⏰ 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). (68)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build min release artifacts on Linux ARM64
- 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 x64
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Lint and validate
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- 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:ciGroup2)
- GitHub Check: linkchecker
- GitHub Check: lighthouse
- GitHub Check: bundle-analyzer
🔇 Additional comments (21)
packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (2)
465-471: LGTM!The role conversion logic correctly handles Bedrock's role constraints (only 'user' and 'assistant'). The fallback to 'user' role when undefined is appropriate.
503-513: LGTM!The filtering logic correctly handles edge cases:
- Empty text blocks are filtered to prevent Bedrock
ValidationException- Non-text blocks (toolUse, toolResult, image) are preserved
- String content is properly converted and filtered
src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx (7)
1-19: LGTM on imports and license header.Imports are well-organized and appropriate for the functionality.
21-24: Internal interface is appropriately scoped.The
DiscoverVisualizationEmbeddableinterface extendsIEmbeddablewith the necessary properties for this action.
26-47: LGTM on LoadingOverlay component.Simple, stateless presentational component with appropriate i18n usage.
49-56: Module augmentation for action type mapping looks correct.The constant export and module augmentation follow the established pattern for UI actions.
58-93: LGTM on class setup and compatibility check.The class structure, grouping, and
isCompatibleimplementation are straightforward and correct.
187-197: The AG-UI protocol message format is correct.The message structure properly implements the AG-UI protocol for binary multimodal content:
type: 'binary',mimeType: 'image/jpeg', anddata(base64) conform to the BinaryInputContent specification, which requires a binary type with mimeType and at least one of id, url, or data.
173-173: TheupdateCurrentMessagesmethod returnsvoidand is explicitly synchronous—it does not return a Promise and internally only uses synchronous operations (sessionStorage.setItem()andJSON.stringify()). No await is necessary, and there is no race condition with subsequent operations.Likely an incorrect or invalid review comment.
src/core/public/chat/types.ts (3)
8-22: LGTM on new input content types.The
TextInputContentandBinaryInputContentinterfaces provide a clean structure for multimodal content. The union typeInputContentallows flexibility for different content types.
44-48: LGTM on BaseMessage content type update.The expanded
contenttype to acceptstring | InputContent[]enables multimodal message support while maintaining backward compatibility with string content.
79-82: LGTM on UserMessage content type update.Aligns with the
BaseMessagechanges to support multimodal content in user messages.src/plugins/chat/common/types.ts (2)
33-48: LGTM on multimodal content schemas.The Zod schemas (
TextInputContentSchema,BinaryInputContentSchema,InputContentSchema) correctly mirror the TypeScript interfaces defined insrc/core/public/chat/types.ts, providing runtime validation for the AG-UI multimodal protocol.
73-76: LGTM on UserMessageSchema content update.The schema correctly allows either a string or an array of
InputContentSchemafor multimodal content support.src/plugins/chat/public/components/message_row.tsx (3)
31-72: LGTM on multimodal content rendering logic.The
renderContentfunction handles the various content formats well:
- String content renders as Markdown
- Array content iterates through blocks (binary for images, text for Markdown)
- Backward compatibility for plain text blocks
- Fallback for unexpected types
The logic is clear and covers the expected cases.
88-91: LGTM on integration of renderContent.The
renderContent()call cleanly replaces the previous direct content rendering.
48-54: mimeType should be validated upstream for defense-in-depth security.While the base64 data URI approach in the
srcattribute is inherently safe from XSS in img tags (browsers won't execute scripts fromdata:URIs even withtext/htmlMIME types), themimeTypefield is not validated upstream—the schema atsrc/plugins/chat/common/types.tsaccepts any string. Add validation to restrict it to known safe image MIME types for robustness.src/plugins/chat/public/components/chat_window.tsx (3)
31-34: LGTM on interface update.The
ChatWindowInstance.sendMessagesignature appropriately accepts an optionalmessagesarray for multimodal content support.
126-137: Potential duplicate messages:additionalMessagesadded to both timeline state andmessagesToSend.The code adds
additionalMessagesto the timeline state (line 129) and also includes them inmessagesToSend(line 133). SincesetTimelineis asynchronous, thetimelinevariable still contains the old state when computingmessagesToSend. This results in:
timeline(old) +additionalMessages→messagesToSendtimeline(old) +additionalMessages→ new state (via setTimeline)This is likely intentional to ensure
messagesToSendincludes the additional messages, but the timeline will then receive theuserMessageagain at line 146, which is correct. However, the flow could be clearer.Consider adding a comment explaining the intentional merge pattern:
// Add additional messages to timeline first if provided const additionalMessages = options?.messages ?? []; if (additionalMessages.length > 0) { setTimeline((prev) => [...prev, ...additionalMessages]); } - // Merge additional messages with current timeline for sending + // Merge additional messages with current timeline for sending + // Note: We use the captured `timeline` (pre-state-update) plus additionalMessages + // to ensure the service receives the complete message list synchronously const messagesToSend = [...timeline, ...additionalMessages];
259-262: LGTM on imperative handle update.The
sendMessagecorrectly forwards bothcontentandmessagesparameters.src/plugins/chat/public/services/chat_service.ts (1)
251-253: LGTM on message forwarding to ChatWindow.The delegation correctly passes both
contentandmessagesto the chat window'ssendMessagemethod.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/plugins/chat/public/components/chat_window.tsx (1)
257-260: Consider adding error handling for undefined ref.The optional chaining
handleSendRef.current?.()silently returnsundefinedif the ref is not set. While this is unlikely given the synchronous assignment at line 173, callers receivingPromise<undefined>instead of the expected result might find it confusing.💡 Optional: Add defensive logging or throw on undefined ref
useImperativeHandle(ref, ()=>({ startNewChat: ()=>handleNewChat(), - sendMessage: async ({content, messages})=>(await handleSendRef.current?.({input:content, messages})) + sendMessage: async ({content, messages})=>{ + if (!handleSendRef.current) { + console.warn('ChatWindow: sendMessage called before initialization'); + return; + } + return handleSendRef.current({input:content, messages}); + } }), [handleNewChat]);src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx (2)
62-66: Consider addingchatavailability to compatibility check.The action checks for
contextProviderbut notchat. Ifchatis unavailable, the action will appear in the menu but fail to send the message (the context will be added but the chat window won't open). Consider whether the action should only be compatible when both services are available.public async isCompatible({ embeddable }: EmbeddableContext) { // Check if this is an explore embeddable and if context provider is available const hasContextProvider = this.contextProvider !== undefined; - return embeddable.type === 'explore' && hasContextProvider; + const hasChatService = this.chat?.chatService !== undefined; + return embeddable.type === 'explore' && hasContextProvider && hasChatService; }
90-96: Add defensive check for base64 extraction.
toDataURL()returns a data URL in the formatdata:image/jpeg;base64,DATA. Whilesplit(',')[1]works for valid data URLs, consider adding a guard in case the canvas rendering fails silently or returns an unexpected format.const canvas = await html2canvas(visEmbeddable.node, { backgroundColor: '#ffffff', logging: false, useCORS: true, }); // Use JPEG format with low quality to save tokens -visualizationBase64 = canvas.toDataURL('image/jpeg', 0.5).split(',')[1]; +const dataUrl = canvas.toDataURL('image/jpeg', 0.5); +visualizationBase64 = dataUrl.split(',')[1] || ''; +if (!visualizationBase64) { + throw new Error('Failed to capture visualization image'); +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/plugins/chat/public/components/chat_window.tsxsrc/plugins/explore/public/actions/ask_ai_embeddable_action.tsx
🔇 Additional comments (7)
src/plugins/chat/public/components/chat_window.tsx (1)
118-144: LGTM! The message merging flow is well-structured.The implementation correctly:
- Handles both UI-driven sends (using internal
inputstate) and programmatic sends (viaoptions.input)- Merges additional context messages with the current timeline for the backend call
- Relies on
chatServiceto consolidate multimodal content into the returneduserMessageThe comment on lines 137-138 clarifies the design decision, which helps maintainability.
src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx (6)
1-20: LGTM!Imports and interface definition are appropriate for the functionality. The
DiscoverVisualizationEmbeddableinterface cleanly extendsIEmbeddableto access the requiredsavedExploreandnodeproperties.
22-29: LGTM!The module augmentation follows the standard pattern for registering new action types in the ui_actions system.
31-44: LGTM!Class properties and grouping configuration are well-defined. The
investigationcategory is appropriate for this AI-powered analysis action.
46-50: LGTM!Constructor properly injects dependencies with appropriate visibility. Optional
contextProviderandchatparameters allow the action to gracefully handle cases where these plugins are unavailable.
52-60: LGTM!Icon and display name methods are correctly implemented with proper i18n support.
143-150: LGTM!Error handling with toast notification is appropriate. The error message extraction handles both
Errorinstances and other thrown values correctly.
| // Add context to the context provider | ||
| if (this.contextProvider) { | ||
| this.chat?.chatService?.updateCurrentMessages([]); | ||
| const contextStore = this.contextProvider.getAssistantContextStore(); | ||
| await contextStore.addContext({ | ||
| id: `visualization-${savedObjectId || embeddable.id}`, | ||
| description: `Visualization: ${title}`, | ||
| value: visualizationContext, | ||
| label: `Visualization: ${title}`, | ||
| categories: ['visualization', 'dashboard', 'chat'], | ||
| }); | ||
| } |
There was a problem hiding this comment.
Message clearing occurs before chat availability is confirmed.
updateCurrentMessages([]) on line 111 clears messages inside the contextProvider block, but before verifying chat?.chatService exists. If chatService is undefined, the context is added and messages are cleared, but no new message is sent—leaving the user with an empty chat and no summary request.
Consider moving the message clearing to after confirming chat is available, or restructuring to ensure atomicity:
// Add context to the context provider
if (this.contextProvider) {
- this.chat?.chatService?.updateCurrentMessages([]);
const contextStore = this.contextProvider.getAssistantContextStore();
await contextStore.addContext({
id: `visualization-${savedObjectId || embeddable.id}`,
description: `Visualization: ${title}`,
value: visualizationContext,
label: `Visualization: ${title}`,
categories: ['visualization', 'dashboard', 'chat'],
});
}
// Send visualization screenshot to chat
if (this.chat?.chatService) {
+ this.chat.chatService.updateCurrentMessages([]);
// Create a message with the visualization image following AG-UI protocol
SuZhou-Joe
force-pushed
the
feature/visualization_summary
branch
from
22d9ee4 to
7c82a08
Compare
SuZhou-Joe
force-pushed
the
feature/visualization_summary
branch
from
7c82a08 to
b1865f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/plugins/explore/public/plugin.test.ts:
- Around line 340-368: Update the two negative tests to assert against the
modified dependency objects rather than the original startDeps: when calling
plugin.start(coreStart, startDepsWithoutChat) assert that
startDepsWithoutChat.uiActions.addTriggerAction was not called with
CONTEXT_MENU_TRIGGER and similarly when calling plugin.start(coreStart,
startDepsWithoutContextProvider) assert that
startDepsWithoutContextProvider.uiActions.addTriggerAction was not called; keep
the existing assertions for AskAIEmbeddableAction and the use of
CONTEXT_MENU_TRIGGER and plugin.start(coreStart, ...) unchanged.
🧹 Nitpick comments (3)
src/plugins/chat/public/components/message_row.tsx (2)
44-44: Consider adding proper typing for content blocks.Using
anytype forblockloses type safety. Consider importing and using theInputContenttype from../../common/typesto ensure type safety and enable IDE assistance.♻️ Suggested improvement
+import type { Message, InputContent } from '../../common/types'; -import type { Message } from '../../common/types'; // Then in the map callback: - {content.map((block: any, index: number) => { + {content.map((block: InputContent, index: number) => {
57-63: Clarify the logic flow for text block handling.The conditions at lines 57-58 and 61-62 have overlapping logic. If a block has
type === 'text'andblock.text, it matches line 57. The condition at line 61 (block.textwithout type check) appears to be a fallback for blocks without an explicittypeproperty.This is fine for backward compatibility, but a brief inline comment would clarify the intent:
📝 Suggested clarification
// Render text content as markdown if (block.type === 'text' && block.text) { return <Markdown key={index} markdown={block.text} openLinksInNewTab={true} />; } - // Handle plain text blocks (for backward compatibility) + // Handle legacy text blocks without explicit type property (backward compatibility) if (block.text) { return <Markdown key={index} markdown={block.text} openLinksInNewTab={true} />; }src/plugins/chat/public/services/chat_service.ts (1)
364-388: Consider avoiding mutation of the input parameter.Line 373 mutates the
messagesparameter directly withmessages = messages.slice(0, -1). While this reassignment doesn't affect the caller's array (sinceslicecreates a new array), the pattern can be confusing. For clarity and to prevent accidental mutations in future refactors, consider using a local variable from the start.The multimodal merging logic itself is correct—it properly appends the text to an existing array-content user message.
Suggested clarification
// Check if the last message in the array is a user message with array content // If so, append the text to the existing content array (for multimodal messages) let userMessage: UserMessage; const lastMessage = messages.length > 0 ? messages[messages.length - 1] : null; const hasArrayContent = lastMessage?.role === 'user' && Array.isArray(lastMessage.content); + + // Create a working copy of messages to avoid confusion about parameter mutation + let messagesToUse = messages; if (hasArrayContent && lastMessage) { // Remove the last message from the array since we'll merge it with the new message - messages = messages.slice(0, -1); + messagesToUse = messages.slice(0, -1); // Append text to the existing content array (preserves order from caller) userMessage = { ...lastMessage, id: this.generateMessageId(), content: [...(lastMessage.content as any[]), { type: 'text', text: content.trim() }], }; } else { // ... }Then use
messagesToUsein therunInput.messagesconstruction on line 406.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
changelogs/fragments/11134.ymlpackage.jsonpackages/osd-agents/src/agents/langgraph/react_graph_nodes.tssrc/core/public/chat/types.tssrc/core/public/mocks.tssrc/plugins/chat/common/types.tssrc/plugins/chat/public/components/chat_window.tsxsrc/plugins/chat/public/components/message_row.tsxsrc/plugins/chat/public/services/chat_service.tssrc/plugins/explore/opensearch_dashboards.jsonsrc/plugins/explore/public/actions/ask_ai_embeddable_action.test.tsxsrc/plugins/explore/public/actions/ask_ai_embeddable_action.tsxsrc/plugins/explore/public/plugin.test.tssrc/plugins/explore/public/plugin.tssrc/plugins/explore/public/types.ts
✅ Files skipped from review due to trivial changes (1)
- changelogs/fragments/11134.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- src/core/public/chat/types.ts
- src/plugins/chat/common/types.ts
- src/plugins/explore/public/actions/ask_ai_embeddable_action.test.tsx
- src/plugins/explore/public/plugin.ts
- src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
src/plugins/chat/public/components/message_row.tsx (1)
src/plugins/opensearch_dashboards_react/public/index.ts (1)
Markdown(45-45)
src/plugins/explore/public/plugin.test.ts (3)
src/plugins/explore/public/types.ts (3)
ExploreStartDependencies(113-128)ExplorePluginStart(78-84)ExploreSetupDependencies(93-108)src/core/public/mocks.ts (1)
coreMock(207-214)src/plugins/explore/public/actions/ask_ai_embeddable_action.tsx (1)
AskAIEmbeddableAction(31-152)
src/plugins/chat/public/components/chat_window.tsx (4)
src/core/public/chat/types.ts (1)
Message(98-103)src/plugins/chat/common/types.ts (1)
Message(12-12)src/core/public/index.ts (1)
Message(442-442)src/core/public/chat/index.ts (1)
Message(11-11)
src/plugins/explore/public/types.ts (3)
src/plugins/chat/server/index.ts (1)
ChatPluginStart(28-28)src/plugins/chat/public/types.ts (1)
ChatPluginStart(16-18)src/plugins/chat/public/index.ts (1)
ChatPluginStart(18-18)
src/plugins/chat/public/services/chat_service.ts (2)
src/core/public/chat/types.ts (1)
UserMessage(79-82)src/core/public/chat/index.ts (1)
UserMessage(15-15)
src/core/public/mocks.ts (2)
src/core/public/chat/chat_service.mock.ts (1)
coreChatServiceMock(50-53)src/core/public/chat/index.ts (1)
coreChatServiceMock(24-24)
⏰ 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). (67)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Lint and validate
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: bundle-analyzer
- GitHub Check: lighthouse
🔇 Additional comments (16)
src/plugins/explore/opensearch_dashboards.json (1)
24-24: LGTM!Adding
chatas an optional plugin is the correct approach. This ensures the Explore plugin can function without the chat feature while enabling the "Ask AI" functionality when the chat plugin is available.src/core/public/mocks.ts (1)
52-52: LGTM!The chat service mock is properly integrated following the established patterns for other service mocks. The import, export, and usage in both
createCoreSetupMockandcreateCoreStartMockare consistent with existing services likekeyboardShortcutServiceMock.Also applies to: 68-68, 97-97, 121-121
src/plugins/explore/public/types.ts (1)
41-41: LGTM!The
ChatPluginStartimport and the optionalchatproperty inExploreStartDependenciesare correctly defined. The optional typing (chat?) properly aligns withchatbeing declared inoptionalPluginsin the plugin manifest.Also applies to: 127-127
src/plugins/chat/public/components/message_row.tsx (1)
31-72: Multimodal content rendering implementation looks good.The
renderContentfunction properly handles:
- Simple string content
- Multimodal arrays with binary (images) and text blocks
- Fallback for unexpected content types
The base64 image rendering with data URIs is appropriate for displaying captured visualizations in the chat context.
Also applies to: 90-90
package.json (1)
270-270: html2canvas version is current with no security advisories.Version 1.4.1 is the latest release (published January 2022). No CVE entries or security vulnerabilities are published for the html2canvas package. The dependency is appropriate and secure for capturing visualization screenshots.
src/plugins/explore/public/plugin.test.ts (5)
1-56: LGTM! Well-organized imports and mocks.The imports cover all necessary dependencies, and the mocks appropriately isolate the unit under test. Mocking
AskAIEmbeddableAction,logActionRegistry,createAskAiAction, andcreateOsdUrlTrackerensures the tests focus on plugin lifecycle behavior without external side effects.
66-183: LGTM! Comprehensive mock factories.The helper functions provide well-structured mocks that cover the essential plugin dependencies. The
createMockStartDepscorrectly includeschatandcontextProvidermocks with the methods needed for testing the AskAIEmbeddableAction integration.
185-241: LGTM! Proper test setup and isolation.The
beforeEachblock correctly initializes all dependencies and the plugin instance. The workspaces mock withpipe,subscribe,getValue, andnextmethods adequately simulates the BehaviorSubject-like interface. TheafterEachcleanup ensures test isolation.
247-318: LGTM! Setup lifecycle tests are comprehensive.The tests verify all key setup behaviors: application registration (explore, logs, traces, metrics), embeddable factory registration, visualization alias registration, and URL forwarding configuration. The use of
expect.objectContainingprovides appropriate flexibility.
370-393: LGTM! Return value and stop lifecycle tests.The tests correctly verify that
startreturns the expected registries and loaders, including the backward-compatiblesavedSearchLoaderalias. The stop lifecycle test ensures graceful cleanup without errors.packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (2)
476-495: LGTM! Binary content transformation for Bedrock images.The conversion from base64 binary blocks to Bedrock's image format is correctly implemented.
Buffer.from(block.data, 'base64')properly converts base64-encoded strings, and in Node.js,Bufferis a subclass ofUint8Array, which satisfies the AWS SDK's expectation forbytes. The format extraction from mimeType with a sensible 'jpeg' fallback is appropriate.
497-513: LGTM! Text content handling and validation.The text block transformation correctly extracts the
textfield into the format Bedrock expects. Filtering empty text blocks (lines 503-509) is essential to prevent Bedrock'sValidationException. The string content fallback (lines 510-513) maintains backward compatibility for non-array content.src/plugins/chat/public/components/chat_window.tsx (3)
31-34: LGTM! Extended sendMessage signature for multimodal support.The updated signature
{content: string; messages?: Message[]}cleanly extends the API to support passing additional messages (e.g., images) alongside the text content. This aligns with the multimodal chat feature requirements.
118-144: LGTM! handleSend properly merges additional messages.The implementation correctly prepares
additionalMessagesand merges them with the timeline before sending. ThechatService.sendMessagehandles the actual content merging, and the returneduserMessage(which may contain merged multimodal content) is added to the timeline. This flow maintains the separation of concerns between timeline management and message construction.
257-260: LGTM! Imperative handle correctly forwards messages.The
useImperativeHandleproperly maps the public API parameters (content,messages) to the internalhandleSendparameters (input,messages), enabling external callers to pass multimodal messages through the ref.src/plugins/chat/public/services/chat_service.ts (1)
250-253: LGTM! Delegation correctly passes multimodal content.The delegation to
ChatWindow.sendMessagenow correctly passes bothcontentandmessages, aligning with the updated interface signature for multimodal support.
| it('should not register Ask AI embeddable action when chat is not available', () => { | ||
| const startDepsWithoutChat = { | ||
| ...startDeps, | ||
| chat: undefined, | ||
| }; | ||
|
|
||
| plugin.start(coreStart, startDepsWithoutChat); | ||
|
|
||
| expect(AskAIEmbeddableAction).not.toHaveBeenCalled(); | ||
| expect(startDeps.uiActions.addTriggerAction).not.toHaveBeenCalledWith( | ||
| CONTEXT_MENU_TRIGGER, | ||
| expect.any(Object) | ||
| ); | ||
| }); | ||
|
|
||
| it('should not register Ask AI embeddable action when contextProvider is not available', () => { | ||
| const startDepsWithoutContextProvider = { | ||
| ...startDeps, | ||
| contextProvider: undefined, | ||
| }; | ||
|
|
||
| plugin.start(coreStart, startDepsWithoutContextProvider); | ||
|
|
||
| expect(AskAIEmbeddableAction).not.toHaveBeenCalled(); | ||
| expect(startDeps.uiActions.addTriggerAction).not.toHaveBeenCalledWith( | ||
| CONTEXT_MENU_TRIGGER, | ||
| expect.any(Object) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Assertions reference the wrong mock object.
In the negative test cases, you're asserting against startDeps.uiActions instead of the modified dependency objects (startDepsWithoutChat and startDepsWithoutContextProvider). While the tests may still pass because startDeps is unmodified, this makes the assertions semantically incorrect and could mask issues if mocks are cleared between tests.
Proposed fix
it('should not register Ask AI embeddable action when chat is not available', () => {
const startDepsWithoutChat = {
...startDeps,
chat: undefined,
};
plugin.start(coreStart, startDepsWithoutChat);
expect(AskAIEmbeddableAction).not.toHaveBeenCalled();
- expect(startDeps.uiActions.addTriggerAction).not.toHaveBeenCalledWith(
+ expect(startDepsWithoutChat.uiActions.addTriggerAction).not.toHaveBeenCalledWith(
CONTEXT_MENU_TRIGGER,
expect.any(Object)
);
});
it('should not register Ask AI embeddable action when contextProvider is not available', () => {
const startDepsWithoutContextProvider = {
...startDeps,
contextProvider: undefined,
};
plugin.start(coreStart, startDepsWithoutContextProvider);
expect(AskAIEmbeddableAction).not.toHaveBeenCalled();
- expect(startDeps.uiActions.addTriggerAction).not.toHaveBeenCalledWith(
+ expect(startDepsWithoutContextProvider.uiActions.addTriggerAction).not.toHaveBeenCalledWith(
CONTEXT_MENU_TRIGGER,
expect.any(Object)
);
});🤖 Prompt for AI Agents
In @src/plugins/explore/public/plugin.test.ts around lines 340 - 368, Update the
two negative tests to assert against the modified dependency objects rather than
the original startDeps: when calling plugin.start(coreStart,
startDepsWithoutChat) assert that
startDepsWithoutChat.uiActions.addTriggerAction was not called with
CONTEXT_MENU_TRIGGER and similarly when calling plugin.start(coreStart,
startDepsWithoutContextProvider) assert that
startDepsWithoutContextProvider.uiActions.addTriggerAction was not called; keep
the existing assertions for AskAIEmbeddableAction and the use of
CONTEXT_MENU_TRIGGER and plugin.start(coreStart, ...) unchanged.
c23c011 to
de07139
Compare
Implement multimodal message support following AG-UI protocol to enable sending visualization screenshots to the Chat assistant. Key Features: - Capture visualization screenshots using html2canvas - Support AG-UI multimodal message format (text + images) - Merge image and text content into single user messages - Convert AG-UI format to Bedrock-compatible image format - Render images alongside text in chat interface Implementation Details: - Added "Ask AI" action to explore visualizations - Extended UserMessage content type to support InputContent arrays - Implemented message merging in ChatService.sendMessage - Added multimodal rendering in MessageRow component - Added Bedrock format conversion in ReactGraphNodes - Fixed duplicate message issue in ChatWindow Testing: - Added comprehensive unit tests for message merging logic - Added tests for multimodal content rendering - Added tests for Bedrock message preparation Technical Details: - Follows AG-UI protocol for vendor-neutral multimodal support - Images sent as base64-encoded JPEG with 0.5 quality - Binary content converted to Buffer for AWS Bedrock SDK - Preserves content order (image first, then text) Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
SuZhou-Joe
force-pushed
the
feature/visualization_summary
branch
from
de07139 to
2ff1a51
Compare
* Fix failed UTs and deps Signed-off-by: Lin Wang <wonglam@amazon.com> * Update src/plugins/chat/server/routes/index.ts Co-authored-by: SuZhou-Joe <suzhou@amazon.com> * Update src/plugins/chat/public/components/chat_window.tsx Co-authored-by: SuZhou-Joe <suzhou@amazon.com> * Update src/plugins/chat/public/components/chat_window.tsx Co-authored-by: SuZhou-Joe <suzhou@amazon.com> * Update src/plugins/chat/public/components/chat_window.tsx Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: Lin Wang <wonglam@amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Co-authored-by: SuZhou-Joe <suzhou@amazon.com>
…oject#11134) * feat: Add visualization screenshot support to Chat Implement multimodal message support following AG-UI protocol to enable sending visualization screenshots to the Chat assistant. Key Features: - Capture visualization screenshots using html2canvas - Support AG-UI multimodal message format (text + images) - Merge image and text content into single user messages - Convert AG-UI format to Bedrock-compatible image format - Render images alongside text in chat interface Implementation Details: - Added "Ask AI" action to explore visualizations - Extended UserMessage content type to support InputContent arrays - Implemented message merging in ChatService.sendMessage - Added multimodal rendering in MessageRow component - Added Bedrock format conversion in ReactGraphNodes - Fixed duplicate message issue in ChatWindow Testing: - Added comprehensive unit tests for message merging logic - Added tests for multimodal content rendering - Added tests for Bedrock message preparation Technical Details: - Follows AG-UI protocol for vendor-neutral multimodal support - Images sent as base64-encoded JPEG with 0.5 quality - Binary content converted to Buffer for AWS Bedrock SDK - Preserves content order (image first, then text) Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * Fix failed UTs and deps (opensearch-project#22) * Fix failed UTs and deps Signed-off-by: Lin Wang <wonglam@amazon.com> * Update src/plugins/chat/server/routes/index.ts Co-authored-by: SuZhou-Joe <suzhou@amazon.com> * Update src/plugins/chat/public/components/chat_window.tsx Co-authored-by: SuZhou-Joe <suzhou@amazon.com> * Update src/plugins/chat/public/components/chat_window.tsx Co-authored-by: SuZhou-Joe <suzhou@amazon.com> * Update src/plugins/chat/public/components/chat_window.tsx Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: Lin Wang <wonglam@amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Co-authored-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Signed-off-by: Lin Wang <wonglam@amazon.com> Co-authored-by: Lin Wang <wonglam@amazon.com> Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>
3 participants
Description
Ask AI Context Menu Action: Right-click action on visualizations in the Explore plugin that:
Issues Resolved
Screenshot
20260113135451999.mp4
Testing the changes
Changelog
Ask AIContext Menu Action to explore visualizationsCheck List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.