Add 'Ask AI' capability to visualizations #11094
Conversation
arjunkumargiri
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
Maosaic,
SuZhou-Joe,
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
arjunkumargiri
force-pushed
the
feature/ask-ai-visualization
branch
from
5b0ff76 to
cdf87bf
Compare
❌ Invalid Prefix For Manual Changeset CreationInvalid description prefix. Found "feat". Only "skip" entry option is permitted for manual commit of changeset files. If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/public/chat/chat_service.ts (1)
134-155: sendMessage is missing imageData parameter in implementationThe type interface
ChatServiceStart.sendMessage(content, messages, imageData?)promises image support, but the core service implementation at line 134 only accepts(content: string, messages: Message[])and delegates asthis.implementation.sendMessage(content, messages)without forwardingimageData. AnyimageDataargument from callers will be silently dropped.
sendMessageWithWindow(lines 143–154) correctly handlesimageDatathrough theoptionsparameter, so onlysendMessageneeds the fix: addimageData?: stringparameter to the method signature and pass it through to the implementation delegation.
🧹 Nitpick comments (10)
src/plugins/chat/public/components/message_row.tsx (1)
34-37: Tighten user-message narrowing and boolean flag for image renderingThe image rendering logic is functionally fine, but you can make the types clearer and avoid the broad cast:
const isUserMessage = message.role === 'user'; const userMessage = isUserMessage ? (message as UserMessage) : undefined; const imageData = userMessage?.imageData; const hasImage = !!imageData;and then:
{hasImage && imageData && ( <div className="messageRow__image"> <EuiImage src={imageData} alt="Visualization screenshot" size="l" hasShadow allowFullScreen /> </div> )}This keeps the discriminated-union semantics explicit and ensures
hasImageis a true boolean without changing behavior.Also applies to: 53-64
src/plugins/chat/public/components/chat_window_loading.test.tsx (1)
6-166: Good coverage of image-related ChatWindow API; consider minor test cleanupThe tests do a nice job validating:
- Ref exposure of
setCapturingImage/setPendingImage.- That
sendMessagepassesimageDatathrough and uses the default prompt when content is empty.A small polish opportunity: in the loading tests you render
ChatWindowtwice in a couple of cases (once without a ref, then again with a ref) even though only the second instance is used. Dropping the unused render would simplify the setup without changing behavior.src/plugins/chat/public/components/chat_window.test.tsx (1)
40-399: ChatWindow tests thoroughly cover new image and loading behaviorThe expanded suite verifies:
- Ref API surface (
startNewChat,sendMessage,setPendingImage,setCapturingImage).- That
sendMessageincludesimageDataand falls back to"Can you analyze this visualization?"when only an image is provided.- Loading-state lifecycle (initial subscription, first response, error, completion).
- Persistence wiring via
getCurrentMessages/updateCurrentMessages.Functionally this aligns well with the new capabilities. There is some overlap with
chat_window_loading.test.tsxaround the ref surface and image send path; if test runtime becomes a concern, those could be consolidated later, but it’s not blocking.src/plugins/chat/public/components/chat_input.test.tsx (1)
190-203: Consider adding a test for sending with both text and image.The current tests cover image-only sends and text-only sends separately. Consider adding a test case that verifies the send button is enabled and
onSendis called when both text input and image are present.🔎 Suggested additional test
it('should enable send button when both input and image are present', () => { const mockImageData = 'data:image/png;base64,test'; const onSend = jest.fn(); render( <ChatInput {...defaultProps} input="test message" pendingImage={mockImageData} onRemoveImage={jest.fn()} onSend={onSend} /> ); const sendButton = screen.getByLabelText('Send message'); expect(sendButton).not.toBeDisabled(); fireEvent.click(sendButton); expect(onSend).toHaveBeenCalled(); });src/plugins/chat/public/services/chat_service.ts (1)
215-225: Silent no-op when chatWindowRef is unavailable.These methods silently do nothing if
chatWindowRef?.currentis null. This appears intentional for the current use case (action may call before ref is established), but callers have no way to know if the operation succeeded.Consider returning a boolean or logging when the ref is unavailable during debugging.
src/plugins/chat/public/components/chat_window.tsx (1)
266-271: useImperativeHandle dependency array may be incomplete.The dependency array only includes
handleNewChat, but the imperative handle referenceshandleSendRef.current,setPendingImage, andsetIsCapturingImage. While the state setters are stable, this could be flagged by exhaustive-deps lint rules.🔎 Consider explicit stable references
useImperativeHandle(ref, ()=>({ startNewChat: ()=>handleNewChat(), sendMessage: async ({content, imageData})=>(await handleSendRef.current?.({input:content, imageData})), setPendingImage: (imageData: string | undefined) => setPendingImage(imageData), setCapturingImage: (isCapturing: boolean) => setIsCapturingImage(isCapturing) - }), [handleNewChat]); + }), [handleNewChat]); // setPendingImage and setIsCapturingImage are stable useState settersAdding a comment clarifies the intentional omission.
src/plugins/visualizations/public/actions/ask_ai_action.tsx (3)
24-25: Unused variables:strategyUsedandselectorUsed.These variables are assigned throughout the function but never read or logged. They appear to be debugging artifacts that should either be removed or used for logging/telemetry.
🔎 Either remove or use for debugging
try { let targetElement: HTMLElement | null = null; - let strategyUsed = ''; // ... strategy code ... - let selectorUsed = ''; // ... selector code ...Or, if useful for debugging:
+ // eslint-disable-next-line no-console + console.debug(`Screenshot captured using strategy: ${strategyUsed}, selector: ${selectorUsed}`); return canvas.toDataURL('image/png');Also applies to: 157-158
276-277: Fragile timing with arbitrary timeouts and retries.The 100ms wait for chat window rendering and the retry loop with 200ms delays are race-condition workarounds. Consider using a more robust approach like waiting for a specific condition or using a callback/promise from the chat service when the window is ready.
🔎 Consider a ready signal instead of arbitrary delays
Instead of:
await new Promise((resolve) => setTimeout(resolve, 100));Consider exposing a
whenReady()promise from the chat service:await chatService.openWindow(); await chatService.whenReady(); // Resolves when ref is establishedThis would eliminate the need for the retry mechanism in lines 307-322 as well.
Also applies to: 307-322
228-249: Large hardcoded visualization type list may become stale.This list needs to be updated whenever new visualization types are added. Consider using a more dynamic approach or moving this to a shared constant.
🔎 Alternative approaches
Option 1: Check for a common trait instead of explicit types:
// Check if embeddable has visualization-like capabilities const hasVisualizationTrait = typeof (embeddable as any).getVisualizationType === 'function' || embeddable.type.includes('vis');Option 2: Move to a shared constant that visualization plugins can register with:
import { SUPPORTED_VISUALIZATION_TYPES } from '../constants'; return SUPPORTED_VISUALIZATION_TYPES.includes(embeddableType);src/plugins/chat/public/components/chat_input.tsx (1)
51-59: ThependingImagevalue is always either a valid base64 data URL fromcanvas.toDataURL('image/png')in the screenshot capture (which guaranteesdata:image/png;base64,...format) or null if capture fails—both of which are safely handled before reaching the EuiImage component. No format validation is needed since the only source is canvas rendering, not user input.
📜 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 (22)
CHANGELOG.mdpackage.jsonpackages/osd-agents/configuration/default_model.jsonpackages/osd-agents/src/agents/langgraph/react_graph_nodes.tssrc/core/public/chat/chat_service.tssrc/core/public/chat/types.tssrc/plugins/chat/common/types.tssrc/plugins/chat/public/components/chat_input.scsssrc/plugins/chat/public/components/chat_input.test.tsxsrc/plugins/chat/public/components/chat_input.tsxsrc/plugins/chat/public/components/chat_input_loading.test.tsxsrc/plugins/chat/public/components/chat_window.test.tsxsrc/plugins/chat/public/components/chat_window.tsxsrc/plugins/chat/public/components/chat_window_loading.test.tsxsrc/plugins/chat/public/components/message_row.scsssrc/plugins/chat/public/components/message_row.tsxsrc/plugins/chat/public/plugin.tssrc/plugins/chat/public/services/chat_service.tssrc/plugins/visualizations/public/actions/ask_ai_action.test.tsxsrc/plugins/visualizations/public/actions/ask_ai_action.tsxsrc/plugins/visualizations/public/actions/index.tssrc/plugins/visualizations/public/plugin.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/plugins/chat/common/types.ts
- src/plugins/chat/public/components/chat_input_loading.test.tsx
- src/plugins/chat/public/components/message_row.scss
- src/plugins/visualizations/public/actions/ask_ai_action.test.tsx
- package.json
- packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts
- src/plugins/chat/public/plugin.ts
- src/core/public/chat/types.ts
- packages/osd-agents/configuration/default_model.json
🧰 Additional context used
🧬 Code graph analysis (7)
src/plugins/chat/public/components/chat_window.test.tsx (2)
src/plugins/chat/public/contexts/chat_context.tsx (1)
ChatProvider(23-33)src/plugins/chat/public/components/chat_window.tsx (2)
ChatWindow(44-46)ChatWindowInstance(28-33)
src/plugins/chat/public/components/message_row.tsx (2)
src/core/public/chat/types.ts (1)
UserMessage(63-67)src/plugins/chat/common/types.ts (1)
UserMessage(16-16)
src/plugins/chat/public/components/chat_window.tsx (4)
src/plugins/chat/public/services/chat_service.ts (1)
setPendingImage(215-219)src/core/public/chat/types.ts (1)
UserMessage(63-67)src/core/public/chat/index.ts (1)
UserMessage(15-15)src/plugins/chat/public/components/chat_input.tsx (1)
ChatInput(24-97)
src/plugins/chat/public/services/chat_service.ts (3)
src/core/public/chat/types.ts (2)
UserMessage(63-67)Message(83-88)src/core/public/chat/index.ts (2)
UserMessage(15-15)Message(11-11)src/core/public/index.ts (1)
Message(442-442)
src/plugins/chat/public/components/chat_input.tsx (1)
src/plugins/chat/public/components/context_pills.tsx (1)
ContextPills(16-83)
src/plugins/chat/public/components/chat_input.test.tsx (1)
src/plugins/chat/public/components/chat_input.tsx (1)
ChatInput(24-97)
src/plugins/visualizations/public/plugin.ts (1)
src/plugins/visualizations/public/actions/ask_ai_action.tsx (3)
ACTION_ASK_AI(12-12)AskAIActionContext(14-16)AskAIAction(191-337)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
9-9: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ 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). (69)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Lint and validate
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: bundle-analyzer
- GitHub Check: lighthouse
- GitHub Check: linkchecker
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (10)
src/plugins/visualizations/public/actions/index.ts (1)
1-6: Barrel export for AskAIAction looks goodCentralizing
AskAIAction,ACTION_ASK_AI, andAskAIActionContextbehind this index keeps the public surface clean and consistent with other actions. No issues from a type or wiring perspective.src/plugins/visualizations/public/plugin.ts (1)
103-110: Ask AI action registration and type wiring look correctThe Ask AI action is cleanly integrated:
- Module augmentation adds
[ACTION_ASK_AI]: AskAIActionContexttoActionContextMapping, so typed contexts line up.- In
start(),AskAIActionis instantiated withcore, registered withuiActions, and attached toCONTEXT_MENU_TRIGGER, which is the right trigger for visualization panel menus.No functional or typing issues spotted here.
Also applies to: 216-220
src/plugins/chat/public/components/chat_input.scss (1)
1-81: Scoped image/loader styles for chat input look solidThe new BEM-style blocks for the input container, loading indicator, and image attachment are well-scoped under
.chatInputand use OUI variables/mixins correctly. No layout or theming red flags from the SCSS alone.src/plugins/chat/public/components/chat_input.tsx (1)
38-94: LGTM! Clean implementation of image attachment UI.The conditional rendering logic is well-structured:
- Loading indicator shown during capture
- Image preview with remove button when image is present
- Placeholder text adapts based on image presence
- Send button correctly enabled for image-only messages
src/plugins/chat/public/components/chat_input.test.tsx (1)
1-269: Good test coverage for the new image functionality.The test suite comprehensively covers:
- Loading indicator visibility states
- Image attachment rendering and removal
- Button state logic with image presence
- Accessibility attributes
src/plugins/chat/public/services/chat_service.ts (2)
367-383: LGTM! Image data properly integrated into message flow.The
imageDataparameter is correctly:
- Added to the method signature
- Included in the
userMessageobject- Passed through the agent run input via the messages array
241-289: LGTM! sendMessageWithWindow correctly propagates imageData.The method properly:
- Accepts
imageDatain the options object- Passes it to
chatWindowRef.current.sendMessage- Includes it in the fallback
userMessage- Falls back to direct
sendMessagewithimageDatasrc/plugins/chat/public/components/chat_window.tsx (2)
115-136: LGTM! Proper handling of image-only messages.The logic correctly:
- Uses
options?.imageData ?? pendingImageto support both programmatic and UI-based image sending- Provides a default message when only an image is present
- Clears
pendingImageafter sending to prevent re-sends
245-256: LGTM! Proper cleanup of image state on new chat.Both
pendingImageandisCapturingImageare correctly reset when starting a new chat, preventing stale image data from persisting.src/plugins/visualizations/public/actions/ask_ai_action.tsx (1)
191-336: Overall implementation is functional with good error handling.The action correctly:
- Checks chat service availability before proceeding
- Shows loading indicator during capture
- Handles screenshot failures gracefully with user-friendly toasts
- Cleans up loading state on errors
| ### 📈 Features/Enhancements | ||
|
|
||
| - Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#TBD](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/TBD)) | ||
|
|
There was a problem hiding this comment.
Fix markdown list indentation and replace TBD PR link
markdownlint is flagging the new bullet’s indentation, and the PR link still uses a placeholder. Suggest tightening both:
- Remove the leading space before the dash to satisfy MD007.
- Replace
TBDwith this PR number (11094) before merge.
Proposed diff
- - Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#TBD](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/TBD))
+- Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#11094](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11094))🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
9-9: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 7 to 10, the bullet has an extra leading space
breaking MD007 and the PR link uses a placeholder; remove the leading space
before the dash so the list item aligns with other bullets and replace the PR
number `TBD` with `11094` in the link URL and text, keeping the rest of the line
unchanged.
| // Strategy 7: Find by title (least reliable, may match wrong visualization) | ||
| if (!targetElement) { | ||
| const title = embeddable.getTitle(); | ||
| if (title) { | ||
| const titleElements = document.querySelectorAll( | ||
| `[title*="${title}"], [aria-label*="${title}"]` | ||
| ); | ||
|
|
||
| for (let i = 0; i < titleElements.length; i++) { | ||
| const titleEl = titleElements[i]; | ||
| const panel = titleEl.closest( | ||
| '.embPanel, .embeddable, [class*="embeddable"]' | ||
| ) as HTMLElement; | ||
| if (panel) { | ||
| targetElement = panel; | ||
| strategyUsed = 'title'; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential CSS selector injection via visualization title.
The title is interpolated directly into CSS attribute selectors without escaping. If a visualization title contains characters like "] or quotes, it could break the selector or cause unexpected matches.
🔎 Suggested fix with CSS.escape
// Strategy 7: Find by title (least reliable, may match wrong visualization)
if (!targetElement) {
const title = embeddable.getTitle();
if (title) {
+ // Escape special characters for CSS selector safety
+ const escapedTitle = CSS.escape(title);
const titleElements = document.querySelectorAll(
- `[title*="${title}"], [aria-label*="${title}"]`
+ `[title*="${escapedTitle}"], [aria-label*="${escapedTitle}"]`
);📝 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.
| // Strategy 7: Find by title (least reliable, may match wrong visualization) | |
| if (!targetElement) { | |
| const title = embeddable.getTitle(); | |
| if (title) { | |
| const titleElements = document.querySelectorAll( | |
| `[title*="${title}"], [aria-label*="${title}"]` | |
| ); | |
| for (let i = 0; i < titleElements.length; i++) { | |
| const titleEl = titleElements[i]; | |
| const panel = titleEl.closest( | |
| '.embPanel, .embeddable, [class*="embeddable"]' | |
| ) as HTMLElement; | |
| if (panel) { | |
| targetElement = panel; | |
| strategyUsed = 'title'; | |
| break; | |
| } | |
| } | |
| } | |
| } | |
| // Strategy 7: Find by title (least reliable, may match wrong visualization) | |
| if (!targetElement) { | |
| const title = embeddable.getTitle(); | |
| if (title) { | |
| // Escape special characters for CSS selector safety | |
| const escapedTitle = CSS.escape(title); | |
| const titleElements = document.querySelectorAll( | |
| `[title*="${escapedTitle}"], [aria-label*="${escapedTitle}"]` | |
| ); | |
| for (let i = 0; i < titleElements.length; i++) { | |
| const titleEl = titleElements[i]; | |
| const panel = titleEl.closest( | |
| '.embPanel, .embeddable, [class*="embeddable"]' | |
| ) as HTMLElement; | |
| if (panel) { | |
| targetElement = panel; | |
| strategyUsed = 'title'; | |
| break; | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/plugins/visualizations/public/actions/ask_ai_action.tsx around lines 114
to 134, the visualization title is interpolated directly into attribute CSS
selectors which can break or be abused if the title contains special characters;
escape the title before using it in querySelectorAll (use CSS.escape(title) and
fall back to a safe manual escaping routine if CSS.escape is unavailable), then
build the selector using the escapedTitle and proceed as before so the selector
cannot be broken by user-controlled characters.
cdf87bf to
c5d1c0c
Compare
❌ Invalid Prefix For Manual Changeset CreationInvalid description prefix. Found "feat". Only "skip" entry option is permitted for manual commit of changeset files. If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description. |
There was a problem hiding this comment.
Actionable comments posted: 0
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/components/chat_window.tsx (1)
183-243: Preserve imageData when resending messages.When resending a user message, the original
imageDatais not passed tochatService.sendMessageor included in the timeline. This means if a user resends a message that originally included a screenshot, the image will be lost.🔎 Proposed fix to preserve imageData on resend
const handleResendMessage = async (message: Message) => { if (isStreaming) return; // Only user messages can be resent if (message.role !== 'user') return; // Find the index of this message in the timeline const messageIndex = timeline.findIndex( (item) => item.id === message.id ); if (messageIndex === -1) return; // Remove this message and everything after it from the timeline const truncatedTimeline = timeline.slice(0, messageIndex); setTimeline(truncatedTimeline); // Clear any streaming state and input setInput(''); setIsStreaming(true); try { + // Preserve imageData from original message if it exists + const originalImageData = message.role === 'user' ? (message as UserMessage).imageData : undefined; + const { observable, userMessage } = await chatService.sendMessage( message.content, - truncatedTimeline + truncatedTimeline, + originalImageData ); // Add user message immediately to timeline const timelineUserMessage: UserMessage = { id: userMessage.id, role: 'user', content: userMessage.content, + imageData: userMessage.imageData, }; setTimeline((prev) => [...prev, timelineUserMessage]);
♻️ Duplicate comments (2)
CHANGELOG.md (1)
7-9: Fix list indentation and replace placeholder PR linkThe new bullet under “Unreleased” still has:
- An extra leading space before
-, which violates MD007 (ul-indent).- A
TBDplaceholder instead of the real PR number.Recommend aligning it with other bullets and using this PR’s ID:
Proposed diff
- - Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#TBD](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/TBD)) +- Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#11094](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11094))Based on static analysis hints and prior review feedback.
src/plugins/visualizations/public/actions/ask_ai_action.tsx (1)
114-134: Apply the CSS.escape fix from the previous review.As noted in the previous review, the visualization title should be escaped before interpolating it into CSS attribute selectors to prevent selector breakage or unintended matches when titles contain special characters like
"]or quotes.
🧹 Nitpick comments (14)
packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (2)
503-526: Avoid redundant Buffer decoding for performance.
Buffer.from(base64Data, 'base64')is called three times (lines 507, 518, 524) for the same data. For large images, this is wasteful. Decode once and reuse:🔎 Proposed fix
if (imageDataMatch) { const [, format, base64Data] = imageDataMatch; + + // Decode base64 once and reuse + const imageBytes = Buffer.from(base64Data, 'base64'); // Add image block to content array const imageBlock = { image: { format, // png, jpeg, gif, webp source: { - bytes: Buffer.from(base64Data, 'base64'), // Convert base64 string to Buffer + bytes: imageBytes, }, }, }; // Add image block to the beginning of content array content.unshift(imageBlock); this.logger.info('✅ Successfully added image block to message', { format, base64Length: base64Data.length, - bufferLength: Buffer.from(base64Data, 'base64').length, + bufferLength: imageBytes.length, totalContentBlocks: content.length, - imageBlockStructure: { - hasImage: true, - hasFormat: !!format, - hasSource: true, - hasBytesBuffer: Buffer.isBuffer(Buffer.from(base64Data, 'base64')), - }, });Additionally, the
imageBlockStructurelogging checks are tautologies (e.g.,Buffer.isBuffer()on a freshly created Buffer is alwaystrue). Consider removing or simplifying if this is temporary debugging code.
499-510: Consider validating image format against Bedrock-supported formats.The comment notes supported formats (png, jpeg, gif, webp), but no validation occurs. If an unsupported format (e.g., bmp, tiff) is passed, the Bedrock API call will fail with a less descriptive error.
🔎 Optional validation
if (imageDataMatch) { const [, format, base64Data] = imageDataMatch; + + const supportedFormats = ['png', 'jpeg', 'jpg', 'gif', 'webp']; + if (!supportedFormats.includes(format.toLowerCase())) { + this.logger.warn('⚠️ Unsupported image format for Bedrock', { + format, + supportedFormats, + }); + // Continue without image or throw - depending on desired behavior + } + + // Normalize 'jpg' to 'jpeg' for Bedrock + const normalizedFormat = format.toLowerCase() === 'jpg' ? 'jpeg' : format.toLowerCase(); // Add image block to content array const imageBlock = { image: { - format, // png, jpeg, gif, webp + format: normalizedFormat,src/plugins/chat/public/components/message_row.tsx (1)
7-7: Image rendering for user messages is correct; consider tightening typingThe new block correctly:
- Only renders the screenshot for user messages with
imageData.- Places the image above the markdown and uses sensible alt text and sizing.
For clarity and type-safety you could (optionally) avoid the blanket cast and make
hasImageexplicitly boolean:Optional refactor
- // Check if this is a user message with image data - const userMessage = message as UserMessage; - const hasImage = message.role === 'user' && userMessage.imageData; + // Check if this is a user message with image data + const isUserMessage = message.role === 'user'; + const userMessage = isUserMessage ? (message as UserMessage) : undefined; + const hasImage = Boolean(isUserMessage && userMessage?.imageData);Also applies to: 9-9, 34-37, 53-64
src/plugins/chat/public/components/chat_window.test.tsx (2)
101-119: Strengthen image-related ref tests and align test name with assertionsThe new ref/image tests validate that:
setPendingImageandsetCapturingImageare exposed onChatWindowInstance.- Calling them does not throw.
- Sending with
{ content, imageData }forwardsimageDatatochatService.sendMessage.- An empty-content + image case maps to a default prompt.
Two improvements would make these tests more robust and less confusing:
Verify UI side-effects, not just method existence
it('should pass isCapturingImage state to ChatInput', ...)andit('should manage pendingImage state correctly', ...)currently only assert that the methods exist and can be called.- Consider asserting on rendered UI (e.g., spinner presence for capturing, attached image thumbnail) to prove the state is actually wired through.
Fix misleading test name
it('should clear capturing state on new chat', ...)only asserts thatmockChatService.newThreadwas called, not that capturing state was cleared.- Either add an assertion that the “capturing” UI is gone after
startNewChat()(e.g., spinner no longer present), or rename the test so it describes the behavior actually being asserted.These are test-only refinements; runtime behavior of the implementation under test isn’t blocked by them.
Also applies to: 149-242
244-357: Loading tests validate subscription wiring; minor cleanup opportunityThe new “loading message functionality” tests correctly:
- Mock different observable behaviors (no emissions, next, error, complete).
- Assert that
chatService.sendMessageis invoked and the returned observable’ssubscribeis called in each case.Minor, optional cleanups:
- In
it('should add loading message to timeline when sending a message', ...), the initialrenderWithContext(<ChatWindow onClose={...} />);before creating therefis redundant and can be dropped to avoid multiple instances.- If you want stronger guarantees, you could also assert on rendered loading-state UI (e.g., presence/absence of a spinner or placeholder message) rather than only
subscribecalls.These are non-blocking test-quality improvements.
src/plugins/chat/public/components/chat_input.test.tsx (1)
1-269: ChatInput tests thoroughly cover new image and loading behaviorThis suite does a good job validating:
- Basic input/change/send/key handling.
- Screenshot capture state (
isCapturingImage) via spinner and text.- Image attachment lifecycle (presence, remove button, placeholder and CSS changes).
- Send-button enable/disable logic with/without text and images.
- Layout mode classes and accessibility labels/roles.
One tiny nit:
it('should show loading indicator above image attachment when both are present', ...)only asserts that both elements render, not their relative order. If ordering becomes important, you could refine that assertion, but it’s not blocking.src/plugins/chat/public/components/chat_input.tsx (1)
7-8: ChatInput image & loading UX is sound; consider gating send during captureThe component updates look solid:
pendingImagedrives an inline thumbnail with optional remove button and a dedicated input CSS class.isCapturingImageshows a clear “Capturing screenshot...” indicator above the input.- Placeholder and send-button disabled logic correctly allow sending when either text is present or an image is attached, and block sends while streaming.
One behavioral nuance to think about: while
isCapturingImageis true andpendingImageis still unset, the send button can be enabled if the user types text:isDisabled = (input.trim().length === 0 && !pendingImage) || isStreaming;If the Ask AI flow assumes the first message after launching it should always include the screenshot, you may want to also gate sending on
!isCapturingImage, e.g.:isDisabled = isStreaming || isCapturingImage || (input.trim().length === 0 && !pendingImage);If allowing users to “bail out” and send text-only while capture is in progress is intentional, the current behavior is fine.
Also applies to: 16-18, 21-21, 28-30, 33-34, 39-93
src/plugins/chat/public/services/chat_service.ts (2)
241-289: Consider adding validation for imageData parameter.The imageData flows correctly through both the delegation and fallback paths. However, consider adding validation to ensure:
- The imageData is a valid base64-encoded data URL
- The size is within acceptable limits to prevent performance issues or backend rejections
🔎 Example validation approach
At the beginning of this method, add:
// Validate imageData if provided if (options?.imageData) { const maxSize = 10 * 1024 * 1024; // 10MB limit const base64Match = options.imageData.match(/^data:image\/(png|jpeg|jpg);base64,(.+)$/); if (!base64Match) { throw new Error('Invalid image data format. Expected base64-encoded PNG or JPEG.'); } const base64Data = base64Match[2]; const sizeInBytes = (base64Data.length * 3) / 4; if (sizeInBytes > maxSize) { throw new Error(`Image size (${Math.round(sizeInBytes / 1024 / 1024)}MB) exceeds maximum allowed size of ${maxSize / 1024 / 1024}MB.`); } }
367-383: Apply the same imageData validation here.The imageData parameter is correctly attached to the userMessage. Apply the same validation suggested for
sendMessageWithWindow(lines 241-289) to ensure data integrity and prevent oversized payloads.src/plugins/chat/public/components/chat_window.tsx (1)
115-172: Verify empty string handling with images.The logic correctly handles sending an image without text by providing a default message. However, line 122's condition
(!messageContent && !imageToSend)would reject an empty string if the user explicitly clears the input after typing. This is likely acceptable behavior, but confirm thatmessageContent || 'Can you analyze this visualization?'(line 125) is the intended fallback logic.Additionally, consider whether pendingImage should be cleared on send errors (currently it's only cleared on line 128 before the try block).
src/plugins/visualizations/public/actions/ask_ai_action.tsx (3)
136-189: Consider logging screenshot errors for debugging.The screenshot capture logic is well-implemented with appropriate html2canvas configuration. However, the catch block silently returns null without logging the error. Consider logging the error to aid debugging while still gracefully handling failures.
🔎 Optional error logging
} catch (error) { + // Log error for debugging without failing the action + // eslint-disable-next-line no-console + console.warn('Screenshot capture failed:', error); return null; }
214-252: Consider making visualization types configurable.The isCompatible check uses a hardcoded list of visualization types. While comprehensive, this list may require maintenance as new visualization types are added. Consider extracting this to a constant that can be easily updated or making it configurable.
🔎 Example refactor
At the top of the file:
// Visualization types that support Ask AI const SUPPORTED_VISUALIZATION_TYPES = [ 'visualization', 'lens', 'map', 'vega', 'timelion', 'input_control_vis', 'metrics', 'tagcloud', 'region_map', 'tile_map', 'histogram', 'line', 'area', 'bar', 'pie', 'metric', 'table', 'gauge', 'goal', 'heatmap', ] as const;Then in isCompatible:
- const visualizationTypes = [ - 'visualization', - 'lens', - // ... rest of types - ]; - - return visualizationTypes.includes(embeddableType); + return SUPPORTED_VISUALIZATION_TYPES.includes(embeddableType);
254-337: LGTM! Comprehensive error handling with retry logic.The execute method properly handles the async screenshot capture flow with appropriate user feedback. The retry mechanism for
setPendingImageis a good solution for timing issues with chat window initialization.Consider making the retry parameters (maxRetries=5, delay=200ms) configurable constants for easier tuning if needed:
const CHAT_WINDOW_READY_MAX_RETRIES = 5; const CHAT_WINDOW_READY_RETRY_DELAY_MS = 200;src/core/public/chat/types.ts (1)
60-67: Consider a branded type for imageData.The imageData field is correctly added to UserMessage as an optional string. For additional type safety, consider using a branded type or more specific type definition to distinguish base64 data URLs from arbitrary strings.
🔎 Example with branded type
// Define a branded type for base64 image data export type Base64ImageData = string & { readonly __brand: 'Base64ImageData' }; export interface UserMessage extends BaseMessage { role: 'user'; content: string; imageData?: Base64ImageData; }This provides compile-time documentation that imageData is not just any string, though it doesn't enforce runtime validation.
📜 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 (22)
CHANGELOG.mdpackage.jsonpackages/osd-agents/configuration/default_model.jsonpackages/osd-agents/src/agents/langgraph/react_graph_nodes.tssrc/core/public/chat/chat_service.tssrc/core/public/chat/types.tssrc/plugins/chat/common/types.tssrc/plugins/chat/public/components/chat_input.scsssrc/plugins/chat/public/components/chat_input.test.tsxsrc/plugins/chat/public/components/chat_input.tsxsrc/plugins/chat/public/components/chat_input_loading.test.tsxsrc/plugins/chat/public/components/chat_window.test.tsxsrc/plugins/chat/public/components/chat_window.tsxsrc/plugins/chat/public/components/chat_window_loading.test.tsxsrc/plugins/chat/public/components/message_row.scsssrc/plugins/chat/public/components/message_row.tsxsrc/plugins/chat/public/plugin.tssrc/plugins/chat/public/services/chat_service.tssrc/plugins/visualizations/public/actions/ask_ai_action.test.tsxsrc/plugins/visualizations/public/actions/ask_ai_action.tsxsrc/plugins/visualizations/public/actions/index.tssrc/plugins/visualizations/public/plugin.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/plugins/chat/public/components/chat_window_loading.test.tsx
- src/plugins/chat/public/components/chat_input_loading.test.tsx
- package.json
- src/plugins/chat/public/components/chat_input.scss
- src/plugins/chat/common/types.ts
- src/plugins/chat/public/components/message_row.scss
- src/plugins/visualizations/public/plugin.ts
- src/plugins/visualizations/public/actions/ask_ai_action.test.tsx
- src/plugins/chat/public/plugin.ts
🧰 Additional context used
🧬 Code graph analysis (8)
src/plugins/chat/public/components/chat_window.test.tsx (2)
src/plugins/chat/public/contexts/chat_context.tsx (1)
ChatProvider(23-33)src/plugins/chat/public/components/chat_window.tsx (2)
ChatWindow(44-46)ChatWindowInstance(28-33)
src/plugins/chat/public/components/chat_window.tsx (5)
src/plugins/chat/public/services/chat_service.ts (1)
setPendingImage(215-219)src/core/public/chat/types.ts (1)
UserMessage(63-67)src/plugins/chat/common/types.ts (1)
UserMessage(16-16)src/core/public/chat/index.ts (1)
UserMessage(15-15)src/plugins/chat/public/components/chat_input.tsx (1)
ChatInput(24-97)
packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (1)
packages/osd-agents/src/utils/logger.ts (1)
error(80-85)
src/plugins/visualizations/public/actions/ask_ai_action.tsx (2)
src/plugins/visualizations/public/actions/index.ts (3)
ACTION_ASK_AI(6-6)AskAIActionContext(6-6)AskAIAction(6-6)src/plugins/ui_actions/public/index.ts (1)
IncompatibleActionError(44-44)
src/plugins/chat/public/services/chat_service.ts (3)
src/core/public/chat/types.ts (2)
UserMessage(63-67)Message(83-88)src/plugins/chat/common/types.ts (2)
UserMessage(16-16)Message(12-12)src/core/public/chat/index.ts (2)
UserMessage(15-15)Message(11-11)
src/plugins/chat/public/components/chat_input.test.tsx (1)
src/plugins/chat/public/components/chat_input.tsx (1)
ChatInput(24-97)
src/core/public/chat/types.ts (3)
src/plugins/chat/common/types.ts (2)
UserMessage(16-16)Message(12-12)src/core/public/chat/index.ts (2)
UserMessage(15-15)Message(11-11)src/core/public/index.ts (1)
Message(442-442)
src/plugins/chat/public/components/message_row.tsx (2)
src/core/public/chat/types.ts (1)
UserMessage(63-67)src/plugins/chat/common/types.ts (1)
UserMessage(16-16)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
9-9: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (13)
packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (1)
552-587: LGTM!The image counting and logging follows the existing pattern for tool use/result counting. This provides good observability for debugging multimodal conversations.
packages/osd-agents/configuration/default_model.json (1)
2-2: Model ID change looks goodThe updated
modelIdis a straightforward config bump with no behavioral implications in this file; no issues from this diff alone.src/plugins/visualizations/public/actions/index.ts (1)
1-6: Ask AI action barrel export is clean and consistentRe-exporting
AskAIAction,ACTION_ASK_AI, andAskAIActionContextfrom a single index simplifies consumption for other plugins and tests; no issues here.src/core/public/chat/chat_service.ts (1)
156-169: Image state setters delegate safely and are backward compatible
setPendingImageandsetCapturingImagecorrectly:
- Check for an implementation and the corresponding method before calling.
- No-op when the chat plugin or image-specific methods are unavailable, which keeps older implementations from breaking.
Callers should treat these as best-effort helpers (not guaranteed to be wired), which seems appropriate for an optional image feature.
src/plugins/chat/public/components/chat_window.test.tsx (2)
44-63: Context wiring for ChatWindow tests is aligned with new APIThe added
mockSuggestedActionsServiceand updatedChatProviderusage ensure tests now reflect the extended context shape; the mockedChatServicesurface matches howChatWindowuses it. No issues here.Also applies to: 65-78
361-383: Persistence integration tests remain valid after ChatWindow API changesThe persistence tests still:
- Restore messages via
getCurrentMessageson mount.- Call
updateCurrentMessagesafter initialization.The use of
as conston persisted messages and the addedonCloseprop in renders don’t change behavior and keep the tests aligned with the updated component API.src/plugins/chat/public/services/chat_service.ts (1)
215-225: LGTM! Clean delegation pattern for image state management.The optional chaining ensures safe no-op behavior when the chat window is not mounted, which is appropriate for these UI state setters.
src/plugins/chat/public/components/chat_window.tsx (4)
28-33: LGTM! Well-defined interface for image support.The ChatWindowInstance interface properly exposes image-related methods and extends sendMessage to accept imageData.
60-61: LGTM! Appropriate state initialization.The pendingImage and isCapturingImage state variables are properly initialized for managing the image attachment flow.
245-256: LGTM! Proper state cleanup.Image state is correctly cleared when starting a new chat, and the remove handler properly clears the pending image.
266-303: LGTM! Proper API exposure and component wiring.The imperative handle correctly exposes the new image-related methods, and ChatInput is properly wired with all necessary props for image handling.
src/plugins/visualizations/public/actions/ask_ai_action.tsx (1)
21-113: LGTM! Comprehensive fallback strategies for DOM discovery.The first six strategies provide robust fallback logic for locating the visualization element. The use of constants and safe string operations prevents injection issues in these strategies.
src/core/public/chat/types.ts (1)
107-174: LGTM! Consistent interface extensions for image support.The ChatServiceInterface and ChatImplementationFunctions are consistently updated with imageData parameters and optional image state methods. The use of optional methods (with
?) is appropriate since not all implementations may support this functionality.
arjunkumargiri
force-pushed
the
feature/ask-ai-visualization
branch
from
c5d1c0c to
4352819
Compare
❌ Invalid Prefix For Manual Changeset CreationInvalid description prefix. Found "feat". Only "skip" entry option is permitted for manual commit of changeset files. If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description. |
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)
packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (1)
449-547: Fix text-block filter and avoid redundant Buffer work in Bedrock message prepTwo small but important points here:
The text-block filter keeps empty text blocks instead of removing them:
content = msg.content.filter((block: any) => !block.text || block.text.trim() !== '');For
{ text: '' },!block.textistrue, so the block is kept. To actually drop empty text blocks while preserving non‑text blocks, invert the condition aroundtextpresence, e.g.:content = msg.content.filter((block: any) => typeof block.text !== 'string' || block.text.trim() !== '' );This aligns with the comment about preventing Bedrock
ValidationExceptions for empty text segments.In the image handling branch you call
Buffer.from(base64Data, 'base64')three times (forbytes,bufferLength, andhasBytesBuffer). Consider computing it once and reusing the same Buffer to avoid unnecessary allocations and decoding in hot paths.These are localized changes but impact robustness of Bedrock requests and logging.
src/plugins/chat/public/components/chat_window.tsx (1)
183-243: Resent user messages drop imageData, which may surprise Ask AI usersIn
handleResendMessage, resending a user message currently ignores any associated image:const { observable, userMessage } = await chatService.sendMessage( message.content, truncatedTimeline ); // ... const timelineUserMessage: UserMessage = { id: userMessage.id, role: 'user', content: userMessage.content, // imageData is not forwarded here };For questions that originally included a visualization screenshot, “Resend” will now send the text alone, losing the image context and thumbnail in the timeline.
To keep behavior consistent with the initial send, consider:
Passing the original image data when resending:
const originalUserMessage = message as UserMessage; const { observable, userMessage } = await chatService.sendMessage( originalUserMessage.content, truncatedTimeline, originalUserMessage.imageData );And propagating it back into the timeline:
const timelineUserMessage: UserMessage = { id: userMessage.id, role: 'user', content: userMessage.content, imageData: userMessage.imageData, };This way, resending an Ask‑AI message preserves the visualization attachment.
♻️ Duplicate comments (1)
CHANGELOG.md (1)
7-10: Fix Unreleased changelog bullet indentation and PR linkThe new bullet under “📈 Features/Enhancements” still has:
- An extra leading space before
-, tripping MD007 (ul-indent).- A placeholder PR reference
[#TBD](.../pull/TBD).Suggest aligning indentation with other bullets and replacing
TBDwith this PR’s number:- - Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#TBD](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/TBD)) +- Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#11094](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11094))This should satisfy markdownlint and keep the changelog consistent.
🧹 Nitpick comments (4)
src/plugins/chat/public/components/message_row.tsx (1)
7-37: User image detection and casting are functionally correctCasting
messagetoUserMessageand guarding all image rendering behindmessage.role === 'user' && userMessage.imageDatagives the right runtime behavior; the image block will only appear for user messages that actually carryimageData. No change required, though you could tighten the type a bit (e.g.,const hasImage = message.role === 'user' && !!(message as UserMessage).imageData) if you want a true boolean.src/plugins/chat/public/components/chat_input.tsx (1)
39-93: Image capture/loading UX is solid; consider optionally gating send during captureThe loading indicator, inline image preview, and dynamic placeholder/send-button enablement make the image attachment flow intuitive, and allowing send when either text or a pending image exists is correct.
If you want to strictly guarantee that “Ask AI” questions always include the captured visualization when capture is in progress, you could optionally also disable the send button while
isCapturingImageis true andpendingImageis still undefined; right now users can type and send a text-only message during capture.src/plugins/chat/public/components/chat_window.tsx (1)
245-271: New chat reset and imperative image APIs are well-scopedClearing
pendingImageandisCapturingImageinhandleNewChat, and exposingsetPendingImage/setCapturingImageviauseImperativeHandle, gives external callers just enough control to drive the screenshot flow without leaking other internals. ThesendMessagewrapper delegating tohandleSendRef.currentalso keeps the behavior consistent between UI- and programmatic sends.src/plugins/chat/public/components/chat_window.test.tsx (1)
183-195: Test does not verify its stated behavior.The test is named "should clear capturing state on new chat" but only verifies that
newThreadwas called. It doesn't actually assert that the capturing state is cleared (e.g., by querying the component state or verifying UI changes).Consider adding an assertion that verifies the state is actually cleared, such as checking that a loading indicator is no longer rendered or that subsequent calls reflect the reset state.
🔎 Suggested improvement
it('should clear capturing state on new chat', () => { const ref = React.createRef<ChatWindowInstance>(); renderWithContext(<ChatWindow ref={ref} onClose={jest.fn()} />); // Set capturing state ref.current?.setCapturingImage(true); // Start new chat should clear the state ref.current?.startNewChat(); expect(mockChatService.newThread).toHaveBeenCalled(); + // TODO: Add assertion to verify capturing state is actually cleared + // e.g., by checking that the loading spinner is not rendered + // or by exposing a getter for the capturing state });
📜 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 (22)
CHANGELOG.mdpackage.jsonpackages/osd-agents/configuration/default_model.jsonpackages/osd-agents/src/agents/langgraph/react_graph_nodes.tssrc/core/public/chat/chat_service.tssrc/core/public/chat/types.tssrc/plugins/chat/common/types.tssrc/plugins/chat/public/components/chat_input.scsssrc/plugins/chat/public/components/chat_input.test.tsxsrc/plugins/chat/public/components/chat_input.tsxsrc/plugins/chat/public/components/chat_input_loading.test.tsxsrc/plugins/chat/public/components/chat_window.test.tsxsrc/plugins/chat/public/components/chat_window.tsxsrc/plugins/chat/public/components/chat_window_loading.test.tsxsrc/plugins/chat/public/components/message_row.scsssrc/plugins/chat/public/components/message_row.tsxsrc/plugins/chat/public/plugin.tssrc/plugins/chat/public/services/chat_service.tssrc/plugins/visualizations/public/actions/ask_ai_action.test.tsxsrc/plugins/visualizations/public/actions/ask_ai_action.tsxsrc/plugins/visualizations/public/actions/index.tssrc/plugins/visualizations/public/plugin.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/core/public/chat/chat_service.ts
- src/plugins/visualizations/public/actions/ask_ai_action.test.tsx
- packages/osd-agents/configuration/default_model.json
- src/plugins/chat/common/types.ts
- src/plugins/chat/public/components/chat_input_loading.test.tsx
- src/plugins/visualizations/public/plugin.ts
- src/plugins/chat/public/services/chat_service.ts
- src/plugins/chat/public/plugin.ts
- src/plugins/chat/public/components/chat_window_loading.test.tsx
- src/plugins/visualizations/public/actions/ask_ai_action.tsx
- src/plugins/chat/public/components/chat_input.scss
- src/plugins/chat/public/components/chat_input.test.tsx
- package.json
🧰 Additional context used
🧬 Code graph analysis (5)
src/plugins/chat/public/components/chat_window.test.tsx (2)
src/plugins/chat/public/contexts/chat_context.tsx (1)
ChatProvider(23-33)src/plugins/chat/public/components/chat_window.tsx (2)
ChatWindow(44-46)ChatWindowInstance(28-33)
src/plugins/chat/public/components/chat_input.tsx (1)
src/plugins/chat/public/components/context_pills.tsx (1)
ContextPills(16-83)
packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (2)
packages/osd-pm/dist/index.js (1)
Buffer(32620-32620)packages/osd-agents/src/utils/logger.ts (1)
error(80-85)
src/plugins/chat/public/components/chat_window.tsx (3)
src/plugins/chat/public/services/chat_service.ts (1)
setPendingImage(215-219)src/core/public/chat/types.ts (1)
UserMessage(63-67)src/plugins/chat/public/components/chat_input.tsx (1)
ChatInput(24-97)
src/plugins/chat/public/components/message_row.tsx (2)
src/core/public/chat/types.ts (1)
UserMessage(63-67)src/plugins/chat/common/types.ts (1)
UserMessage(16-16)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
9-9: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (16)
src/plugins/visualizations/public/actions/index.ts (1)
1-6: Ask AI action re-export looks correctRe-exporting
AskAIAction,ACTION_ASK_AI, andAskAIActionContextfrom the actions barrel matches the PR’s intent and keeps the public surface cohesive. No changes needed.packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (1)
552-587: Image counting and logging behavior is soundThe additional
imageCountaggregation and per‑message image logging correctly piggyback on the prepared content blocks and will help debug multimodal conversations without affecting Bedrock payloads. No changes needed here.src/plugins/chat/public/components/message_row.scss (1)
45-47: Image block spacing is reasonableAdding
.messageRow__image { margin-bottom: 12px; }cleanly separates the image thumbnail from the markdown content and fits the existing spacing scale. Looks good as-is.src/plugins/chat/public/components/message_row.tsx (1)
53-64: Inline image rendering in message rows is well-integratedPlacing the visualization screenshot above the markdown body inside the same
EuiPanelmaintains alignment with existing styling and leverages the new.messageRow__imagespacing. Alt text and full-screen support are set, so users get a clear preview and can inspect the visualization if needed.src/plugins/chat/public/components/chat_input.tsx (1)
12-22: ChatInput API extension for image and capture state is coherentAdding
pendingImage,isCapturingImage, andonRemoveImagetoChatInputPropscleanly exposes all the state needed for the Ask AI flow without overloading existing props. Shapes line up with howChatWindowdrives them.src/plugins/chat/public/components/chat_window.tsx (3)
60-136: Message sending with optional image data is wired correctly
handleSend’s extended signature and logic:
- Accepts optional
{ input, imageData }, falling back to componentinputandpendingImage.- Allows sending when there is either text or an image, with a sensible default prompt when only an image is present.
- Clears
inputandpendingImage, setsisStreaming, and forwardsimageToSendintochatService.sendMessage.This matches the intended Ask AI behavior and keeps the normal text-only path intact.
138-145: Timeline user messages correctly retain imageDataIncluding
imageData: userMessage.imageDatawhen pushing the user message intotimelineensures downstream renderers (likeMessageRow) can show the attached screenshot alongside the text. This is essential for multimodal history and looks correct.
291-303: Plumbing of image state into ChatInput is consistentPassing
pendingImage,isCapturingImage, andonRemoveImagedirectly intoChatInputensures the footer UI always reflects the current window state for image attachments. This matches the new props inChatInputand keeps the wiring straightforward.src/core/public/chat/types.ts (3)
60-67: UserMessage imageData extension matches runtime usageAdding
imageData?: stringtoUserMessageprovides a typed home for the base64 screenshot your UI is already sending and rendering. It’s optional, so existing text-only callers remain valid.
136-148: ChatServiceInterface image support and helper methods are consistentExtending
sendMessage/sendMessageWithWindowwith an optionalimageDataparameter, plus optionalsetPendingImage/setCapturingImagehelpers, matches the ChatWindow and chat_service wiring and keeps the public API cohesive for both direct sends and window-driven flows.
153-174: Implementation function signatures stay in sync with the public interfaceMirroring the same
imageDataparameters and optional image helper callbacks inChatImplementationFunctionsensures plugin implementations stay aligned with the core interface. This symmetry should prevent type drift between core and plugin layers.src/plugins/chat/public/components/chat_window.test.tsx (5)
44-78: LGTM!The mock setup for
suggestedActionsServiceand the updatedrenderWithContextwrapper correctly align with theChatProviderinterface, ensuring tests have access to both required services.
101-119: LGTM!The new tests for
setPendingImageandsetCapturingImagefollow the established pattern for ref method verification and correctly validate the public API exposure.
274-357: LGTM!The loading message tests cover the key observable lifecycle scenarios (response received, error, completion) and verify the integration with
chatService.sendMessage. The mock observable pattern is consistent and correctly simulates async behavior.
358-400: LGTM!The persistence integration tests are correctly updated with the
onCloseprop, and theas constassertions on lines 361-362 ensure proper literal typing for the message roles.
197-241: LGTM!Good test coverage for the image data flow. The tests verify both the imageData passthrough to
sendMessageand the default message behavior when only an image is provided without text content.
github-actions
bot
added
Skip-Changelog
Signed-off-by: Arjun kumar Giri <[email protected]>
- Pulled latest yarn.lock from origin/main to resolve conflicts - Regenerated yarn.lock with yarn install to ensure consistency Signed-off-by: Arjun kumar Giri <[email protected]>
Signed-off-by: Kyle Hounslow <[email protected]> Signed-off-by: Arjun kumar Giri <[email protected]>
…oject#11084) * use kbn handlebars compileAST to avoid violating strict CSP, and exlude in webpack for use Signed-off-by: Paul Sebastian <[email protected]> * use kbn handlebars for server side usage and remove handlebars entirely Signed-off-by: Paul Sebastian <[email protected]> * Changeset file for PR opensearch-project#11084 created/updated * Remove 'single error about inline script' message Signed-off-by: Paul Sebastian <[email protected]> * update yarn lock file Signed-off-by: Paul Sebastian <[email protected]> --------- Signed-off-by: Paul Sebastian <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Signed-off-by: Arjun kumar Giri <[email protected]>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-ci-bot <[email protected]> Co-authored-by: Sayali Gaikawad <[email protected]> Signed-off-by: Arjun kumar Giri <[email protected]>
Signed-off-by: Joshua Li <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Signed-off-by: Arjun kumar Giri <[email protected]>
* Upgrade oui version to 1.22 Signed-off-by: Joey Liu <[email protected]> * Changeset file for PR opensearch-project#11042 created/updated * Upgrade to 1.22.1 Signed-off-by: Joey Liu <[email protected]> --------- Signed-off-by: Joey Liu <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Signed-off-by: Arjun kumar Giri <[email protected]>
…pensearch-project#11086) * chore(deps): update dependency @modelcontextprotocol/sdk to v1.24.0 Signed-off-by: Lin Wang <[email protected]> * Changeset file for PR opensearch-project#11086 created/updated --------- Signed-off-by: Lin Wang <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Signed-off-by: Arjun kumar Giri <[email protected]>
…e StandardAxes (opensearch-project#10935) --------- Signed-off-by: Yulong Ruan <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Signed-off-by: Arjun kumar Giri <[email protected]>
* feat: support flat_object field type Signed-off-by: Hailong Cui <[email protected]> * Changeset file for PR opensearch-project#11085 created/updated --------- Signed-off-by: Hailong Cui <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Signed-off-by: Arjun kumar Giri <[email protected]>
Setup ECharts for discover visualizations, this PR added a flag `__DEVELOPMENT__.discover.vis.render: vega|echarts`, when set to echarts, the visualizations will be rendered by echarts. --------- Signed-off-by: Yulong Ruan <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Signed-off-by: Arjun kumar Giri <[email protected]>
* Add back handlebars Signed-off-by: Lin Wang <[email protected]> * Changeset file for PR opensearch-project#11105 created/updated --------- Signed-off-by: Lin Wang <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Signed-off-by: Arjun kumar Giri <[email protected]>
…h-project#11106) --------- Signed-off-by: Qxisylolo <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Signed-off-by: Arjun kumar Giri <[email protected]>
* refactor pivot and aggregate function fix empty pivot field not displayed correctly refactor createBarSeries to unified the stacked bar series creation Signed-off-by: Yulong Ruan <[email protected]> * revert version update Signed-off-by: Yulong Ruan <[email protected]> * remove aggregateByTime which is replaced by aggregate function Signed-off-by: Yulong Ruan <[email protected]> --------- Signed-off-by: Yulong Ruan <[email protected]> Signed-off-by: Arjun kumar Giri <[email protected]>
arjunkumargiri
force-pushed
the
feature/ask-ai-visualization
branch
from
f921f95 to
2b6e37c
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/plugins/vis_type_timeseries/public/application/components/lib/tick_formatter.js (1)
31-40: Addstrict: trueoption tocompileASTcall for consistency.Line 40 uses
compileASTwith only{ knownHelpersOnly: true }, but the same method is used inreplace_vars.js(line 37) with both{ strict: true, knownHelpersOnly: true }. For consistency across the codebase, add thestrict: trueoption:const render = Handlebars.compileAST(template, { strict: true, knownHelpersOnly: true });Legacy code in
app_bootstrap.jsalso usesstrict: truewith Handlebars compilation, establishing this as the pattern in the codebase.packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (1)
449-590: Tighten text-block filtering and reuse image buffer to match intentTwo small correctness/efficiency issues here:
- Empty text filtering doesn’t quite match the comment
content = msg.content.filter((block: any) => !block.text || block.text.trim() !== '');
- For
block.text === '',!block.textistrue, so the block is kept, even though it’s “empty text”.- If a block has a non‑string
textfield,.trim()will throw.Given the comment about preventing
ValidationException, it’s safer to explicitly gate on strings:Suggested fix for text-block filtering
- if (Array.isArray(msg.content)) { - // Filter out empty text blocks to prevent ValidationException - content = msg.content.filter((block: any) => !block.text || block.text.trim() !== ''); - } else { - // Convert string content to array format - const textContent = msg.content || ''; - if (textContent.trim() !== '') { - content = [{ text: textContent }]; - } else { - content = []; - } - } + if (Array.isArray(msg.content)) { + // Filter out text blocks that are only whitespace, but keep non-text blocks (toolUse, image, etc.) + content = msg.content.filter((block: any) => { + if (typeof block.text === 'string') { + return block.text.trim() !== ''; + } + return true; + }); + } else { + // Convert primitive/string content to array format + const textContent = String(msg.content ?? ''); + if (textContent.trim() !== '') { + content = [{ text: textContent }]; + } else { + content = []; + } + }
- Avoid repeated
Buffer.fromallocations for the same imageInside the image handling block you call
Buffer.from(base64Data, 'base64')three times (for the payload, length logging, andisBuffercheck). You can compute it once:Suggested buffer reuse in image handling
- if (imageDataMatch) { - const [, format, base64Data] = imageDataMatch; + if (imageDataMatch) { + const [, format, base64Data] = imageDataMatch; + const imageBytes = Buffer.from(base64Data, 'base64'); @@ - bytes: Buffer.from(base64Data, 'base64'), // Convert base64 string to Buffer + bytes: imageBytes, // Convert base64 string to Buffer once @@ - base64Length: base64Data.length, - bufferLength: Buffer.from(base64Data, 'base64').length, + base64Length: base64Data.length, + bufferLength: imageBytes.length, @@ - hasBytesBuffer: Buffer.isBuffer(Buffer.from(base64Data, 'base64')), + hasBytesBuffer: Buffer.isBuffer(imageBytes),These changes align behavior with the comments, reduce the chance of Bedrock validation issues from empty text blocks, and avoid unnecessary allocations on the hot path.
src/plugins/explore/public/components/query_panel/query_panel_widgets/dataset_select/dataset_select.test.tsx (1)
125-141: Update mockDEFAULT_DATA.SET_TYPESvalues to match actual constants.The mock defines
INDEX: 'INDEX'andINDEX_PATTERN: 'index_pattern', but the actual constants insrc/plugins/data/common/constants.tsuseINDEX: 'INDEXES'andINDEX_PATTERN: 'INDEX_PATTERN'. Update lines 129-130 to reflect the real values so tests validate against actual behavior.
♻️ Duplicate comments (4)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/06/rule_matching_vis_line.spec.js (1)
51-53: Selector updates are consistent with the area chart test.The changes here mirror those in
rule_matching_vis_area.spec.js. The same verification concerns apply (see previous comment).CHANGELOG.md (1)
7-10: Fix list indentation and replace placeholder PR linkThere are two small issues here:
- Line 9 has an extra leading space before
-, which violates MD007 and is inconsistent with the rest of the file.- The PR reference still uses
TBDinstead of this PR number (11094).You can fix both with:
Proposed diff
-### 📈 Features/Enhancements - - - Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#TBD](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/TBD)) +### 📈 Features/Enhancements + +- Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#11094](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11094))src/plugins/chat/public/components/chat_window.test.tsx (1)
244-272: Orphaned render call on line 246.The
renderWithContext(<ChatWindow onClose={jest.fn()} />)on line 246 creates a component that is never used. The test then sets up mocks and renders again with a ref on lines 261-262. This appears to be a copy-paste artifact that should be removed.🔎 Proposed fix
describe('loading message functionality', () => { it('should add loading message to timeline when sending a message', async () => { - renderWithContext(<ChatWindow onClose={jest.fn()} />); - // Mock the sendMessage to return a controllable observable const loadingObservable = { subscribe: jest.fn(() => { // Don't call next immediately to simulate loading state return { unsubscribe: jest.fn() }; }), }; mockChatService.sendMessage.mockResolvedValue({ observable: loadingObservable, userMessage: { id: 'user-1', content: 'test', role: 'user' }, }); const ref = React.createRef<ChatWindowInstance>(); renderWithContext(<ChatWindow ref={ref} onClose={jest.fn()} />);src/plugins/visualizations/public/actions/ask_ai_action.tsx (1)
114-134: Potential CSS selector injection via visualization title.The title is interpolated directly into CSS attribute selectors without escaping. If a visualization title contains characters like
"]or quotes, it could break the selector or cause unexpected matches.🔎 Suggested fix with CSS.escape
// Strategy 7: Find by title (least reliable, may match wrong visualization) if (!targetElement) { const title = embeddable.getTitle(); if (title) { + // Escape special characters for CSS selector safety + const escapedTitle = CSS.escape(title); const titleElements = document.querySelectorAll( - `[title*="${title}"], [aria-label*="${title}"]` + `[title*="${escapedTitle}"], [aria-label*="${escapedTitle}"]` );
🧹 Nitpick comments (26)
src/plugins/data/public/antlr/promql/code_completion.ts (1)
47-54: Consider extracting the inline structural type.The inline structural type for the Prometheus resource client is verbose and makes the code harder to read. Consider extracting it to a named interface for better clarity and potential reusability.
🔎 Proposed refactor to extract the type
Add this interface near the top of the file (after existing interfaces):
+interface PrometheusResourceClient { + getMetricMetadata: ( + dataSourceId?: string + ) => Promise<Record<string, Array<{ type: string; help: string }>>>; + getLabels: (dataSourceId: string, metric?: string) => Promise<string[]>; + getLabelValues: (dataSourceId: string, label: string, metric?: string) => Promise<string[]>; +}Then simplify the get call:
- const prometheusResourceClient = services.data.resourceClientFactory.get<{ - getMetricMetadata: ( - dataSourceId?: string - ) => Promise<Record<string, Array<{ type: string; help: string }>>>; - getLabels: (dataSourceId: string, metric?: string) => Promise<string[]>; - getLabelValues: (dataSourceId: string, label: string, metric?: string) => Promise<string[]>; - }>('prometheus'); + const prometheusResourceClient = services.data.resourceClientFactory.get<PrometheusResourceClient>('prometheus'); if (!prometheusResourceClient) throw new Error('Prometheus resource client not found.');src/plugins/data/common/osd_field_types/types.ts (1)
101-101: Minor: verify the comment update is intentional.The comment reference was updated but it's unclear if this change is necessary or adds value. If it's just clarifying the source of PPL field types, it's fine to keep.
src/plugins/chat/public/components/chat_window.tsx (1)
183-216: Preserve imageData when resending a user messageIn
handleResendMessage, user messages that originally included a screenshot are resent with text only:const { observable, userMessage } = await chatService.sendMessage( message.content, truncatedTimeline );To keep the resend behavior consistent for Ask‑AI image flows, it would be better to forward any
imageDataon the message as well:Suggested change
- try { - const { observable, userMessage } = await chatService.sendMessage( - message.content, - truncatedTimeline - ); + try { + const { observable, userMessage } = await chatService.sendMessage( + message.content, + truncatedTimeline, + (message as UserMessage).imageData + );This way, “Resend message” truly replays the original user input, including any attached visualization screenshot.
src/plugins/visualizations/public/actions/ask_ai_action.tsx (3)
71-89: Consider adding type guards for safer property access.The heavy use of
(embeddable as any)to access undocumented properties likegetRoot,domNode, and container methods is fragile. While the try-catch handles failures gracefully, this could break silently with future embeddable API changes.Consider documenting which embeddable implementations support these properties, or adding runtime type checks:
const hasGetRoot = (e: unknown): e is { getRoot: () => unknown } => typeof (e as any)?.getRoot === 'function';
226-252: Hardcoded visualization types may require maintenance.The
visualizationTypesarray is a static allowlist. New visualization types added to the platform won't automatically be supported by this action.Consider whether there's a more dynamic approach, such as:
- Checking for a common base type or interface
- Using a registry pattern where visualization plugins register themselves as AI-compatible
- Falling back to the current allowlist if no dynamic mechanism exists
276-277: Timing-based wait for UI readiness.The 100ms delay is a race condition mitigation but may be insufficient on slower systems or excessive on fast ones. The retry mechanism at lines 308-320 partially addresses this, but consider whether a more deterministic approach (e.g., polling for ref availability) would be more reliable.
src/plugins/explore/public/components/visualizations/bar/bar_exclusive_vis_options.tsx (1)
84-108: Remove commented-out code and unused prop.The
shouldDisableUseThresholdColorprop is defined in the interface and destructured but no longer used. The commented-out block (lines 84-96) should be removed rather than left as dead code.🔎 Proposed fix
Remove the commented-out code block and consider removing
shouldDisableUseThresholdColorfrom the interface and function parameters if it's no longer needed:- {/* {!shouldDisableUseThresholdColor && ( - <EuiFormRow> - <EuiSwitch - compressed - label={i18n.translate('explore.vis.bar.useThresholdColor', { - defaultMessage: 'Use threshold colors', - })} - data-test-subj="useThresholdColorButton" - checked={useThresholdColor ?? false} - onChange={(e) => onUseThresholdColorChange(e.target.checked)} - /> - </EuiFormRow> - )} */} - <EuiFormRow>src/plugins/explore/public/components/visualizations/bar/to_expression.ts (2)
59-106: Consider extracting duplicated axis extraction logic.The axis configuration extraction and field determination logic (lines 62-82) is repeated nearly identically in
createTimeBarChart,createGroupedTimeBarChart, andcreateStackedBarSpec. Consider extracting this into a shared helper function to reduce duplication and maintenance burden.🔎 Example helper extraction
const extractAxisFields = ( axisConfig: ReturnType<typeof getSwappedAxisRole>, styles: BarChartStyle ): { categoryField: string; valueField: string } => { const { xAxis, yAxis } = axisConfig; if (!xAxis || !yAxis) { throw Error('Missing axis config for Bar chart'); } if (xAxis.schema === VisFieldType.Categorical) { return { categoryField: xAxis.column, valueField: yAxis.column }; } else if (yAxis.schema === VisFieldType.Categorical) { return { categoryField: yAxis.column, valueField: xAxis.column }; } return { categoryField: styles.switchAxes ? yAxis.column : xAxis.column, valueField: styles.switchAxes ? xAxis.column : yAxis.column, }; };
390-392: Ensure consistent error handling for missing required fields.The error for missing
colorField(line 390-392) throws a genericError, but similar validation at lines 66-68 throws withError('Missing axis config for grouped time bar chart'). Consider using a consistent error type or message format across all validation points.src/plugins/explore/public/components/visualizations/utils/utils.ts (2)
458-465: Handle empty thresholds array edge case.If an empty array is passed to
convertThresholds, it returns an empty array which is fine. However, the function assumes the input is sorted by value. Consider documenting this assumption or adding sorting logic.🔎 Proposed fix with sorting
export const convertThresholds = (thresholds: Threshold[]) => { - return thresholds.map((t, i) => ({ + const sorted = [...thresholds].sort((a, b) => a.value - b.value); + return sorted.map((t, i) => ({ min: t.value, - max: i === thresholds.length - 1 ? Infinity : thresholds[i + 1].value, - + max: i === sorted.length - 1 ? Infinity : sorted[i + 1].value, color: t.color, })); };
523-530: Avoid repeatednew Date()calls for consistency.The time marker creates multiple
Dateobjects (lines 525 and 528) which could have slightly different timestamps if called across a second boundary. Consider capturing the date once.🔎 Proposed fix
if (addTimeMarker) { + const now = new Date(); data.push({ - xAxis: new Date(), + xAxis: now, itemStyle: { color: 'red' }, lineStyle: { type: 'dashed' }, - label: { formatter: new Date().toISOString() }, + label: { formatter: now.toISOString() }, }); }src/plugins/explore/public/components/visualizations/vega_render.tsx (1)
50-56: Consider using a more stable key generation approach.Using
JSON.stringify(searchContext)in the key can lead to unnecessary re-renders due to unstable property ordering and potential performance issues with large objects.🔎 Suggested improvement
Consider using a more stable approach:
+import { useMemo } from 'react'; export const VegaRender = ({ searchContext, ExpressionRenderer, onSelectTimeRange, spec, }: Props) => { + const stableKey = useMemo(() => { + // Create a stable hash or use specific fields + return `${searchContext?.timeRange?.from}-${searchContext?.timeRange?.to}-${expression}`; + }, [searchContext, expression]); + // ... rest of component return ( <ExpressionRenderer - key={JSON.stringify(searchContext) + expression} + key={stableKey} expression={expression} searchContext={searchContext} onEvent={onExpressionEvent} /> ); };src/plugins/explore/public/components/visualizations/echarts_render.tsx (2)
14-16: Simplify the instance management.Both
instancestate andinstanceRefare maintained, butinstanceRefalone should be sufficient. ThesetInstance(echartsInstance)call triggers an extra render without clear benefit.🔎 Proposed simplification
export const EchartsRender = ({ spec }: Props) => { - const [instance, setInstance] = useState<echarts.ECharts | null>(null); const containerRef = useRef<HTMLDivElement | null>(null); const instanceRef = useRef<echarts.ECharts | null>(null); // ... ResizeObserver setup ... useEffect(() => { if (containerRef.current) { const echartsInstance = echarts.init(containerRef.current); instanceRef.current = echartsInstance; containerResizeObserver.observe(containerRef.current); - setInstance(echartsInstance); } return () => { containerResizeObserver.disconnect(); if (instanceRef.current) { instanceRef.current.dispose(); instanceRef.current = null; } }; }, [containerResizeObserver]); useEffect(() => { - if (instance && spec) { - instance.setOption( + if (instanceRef.current && spec) { + instanceRef.current.setOption( { ...spec, grid: { top: 60, bottom: 60, left: 60, right: 60 }, }, { notMerge: true } ); } - }, [spec, instance]); + }, [spec]);Also applies to: 29-32
44-54: Consider making grid values configurable.The grid spacing is hardcoded to 60px on all sides, which overrides any grid settings from the spec. Consider making these values configurable via props or respecting values from the spec when provided.
💡 Suggested enhancement
interface Props { spec: echarts.EChartsOption; + gridPadding?: { top?: number; bottom?: number; left?: number; right?: number }; } -export const EchartsRender = ({ spec }: Props) => { +export const EchartsRender = ({ spec, gridPadding }: Props) => { // ... existing code ... useEffect(() => { if (instance && spec) { + const defaultGrid = { top: 60, bottom: 60, left: 60, right: 60 }; instance.setOption( { ...spec, - grid: { top: 60, bottom: 60, left: 60, right: 60 }, + grid: { ...defaultGrid, ...spec.grid, ...gridPadding }, }, { notMerge: true } ); } }, [spec, instance, gridPadding]);src/plugins/explore/public/components/visualizations/utils/axis.ts (1)
18-40: Refactor to avoid sparse arrays and improve type safety.The current implementation has several issues:
- Sparse array risk: Using hard-coded indices (results[0], results[1], results[2]) can create sparse arrays if roles are processed in unexpected orders.
- Type safety:
Object.keys(axisColumnMappings)loses type information, requiring type assertion at line 19.- Code duplication: The default config assignment logic is duplicated in lines 23-29 and 31-37.
🔎 Proposed refactor using a role-to-index map
export const getAxisConfigByColumnMapping = ( axisColumnMappings: Partial<Record<AxisRole, VisColumn>>, standardAxes: StandardAxes[] = [] ) => { + const roleToIndex: Record<AxisRole, number> = { + [AxisRole.X]: 0, + [AxisRole.Y]: 1, + [AxisRole.Y_SECOND]: 2, + }; + + const roleToDefault: Record<AxisRole, StandardAxes> = { + [AxisRole.X]: DEFAULT_X_AXIS_CONFIG, + [AxisRole.Y]: DEFAULT_Y_AXIS_CONFIG, + [AxisRole.Y_SECOND]: DEFAULT_Y_2_AXIS_CONFIG, + }; + const results: StandardAxes[] = []; - Object.keys(axisColumnMappings).forEach((role) => { - const column = axisColumnMappings[role as AxisRole]; + + (Object.keys(axisColumnMappings) as AxisRole[]).forEach((role) => { + const column = axisColumnMappings[role]; if (column) { const found = standardAxes.find((config) => config.axisRole === role); - if (found) { - if (role === AxisRole.X) { - results[0] = found; - } else if (role === AxisRole.Y) { - results[1] = found; - } else if (role === AxisRole.Y_SECOND) { - results[2] = found; - } - } else { - if (role === AxisRole.X) { - results[0] = DEFAULT_X_AXIS_CONFIG; - } else if (role === AxisRole.Y) { - results[1] = DEFAULT_Y_AXIS_CONFIG; - } else if (role === AxisRole.Y_SECOND) { - results[2] = DEFAULT_Y_2_AXIS_CONFIG; - } - } + const index = roleToIndex[role]; + results[index] = found ?? roleToDefault[role]; } }); + return results.filter(Boolean); };src/plugins/explore/public/components/data_table/metrics_data_table.tsx (1)
52-53: Consider extracting the magic string 'Time' as a constant.The column name 'Time' is hard-coded here. If this is a well-known field name in the metrics context, consider extracting it as a named constant for better maintainability.
🔎 Optional refactor
At the top of the file:
+const TIME_COLUMN_ID = 'Time';Then update the reference:
- if (columnId === 'Time' && typeof value === 'number') { + if (columnId === TIME_COLUMN_ID && typeof value === 'number') {src/plugins/explore/public/components/data_table/metrics_data_table.test.tsx (1)
62-190: Strengthen test assertions to verify actual behavior.The current tests are primarily smoke tests that only verify the component renders without crashing. They don't validate:
- Actual cell content rendering
- Correct date formatting in displayed cells
- Pagination controls and behavior
- Column visibility toggles
- Placeholder rendering for missing values
Lines 70-77, 103-104, and 165-166 contain comments describing expected behavior that isn't actually asserted.
🔎 Example of stronger assertions
it('renders data grid with timestamp data for formatting', () => { const { container } = render(<MetricsDataTable searchResult={mockSearchResult} />); expect(container.querySelector('.euiDataGrid')).toBeInTheDocument(); - // Verify the component receives timestamp data that will be formatted using dateFormat setting - expect(mockSearchResult.instantHits?.hits[0]._source.Time).toBe(1638316800000); + // Verify timestamp is actually formatted in the rendered output + expect(screen.getByText(/2021-11-30/)).toBeInTheDocument(); }); it('renders with correct pagination settings', () => { const { container } = render(<MetricsDataTable searchResult={mockSearchResult} />); expect(container.querySelector('.euiDataGrid')).toBeInTheDocument(); - // Default page size is 50 + expect(screen.getByText(/Rows per page/i)).toBeInTheDocument(); + expect(screen.getByText(/50/)).toBeInTheDocument(); });src/plugins/explore/public/components/visualizations/constants.ts (1)
71-87: Consider making base config more generic.The
DEFAULT_AXIS_CONFIGhas X-axis-specific values (position: BOTTOM, axisRole: X) that are then overridden for Y axes. While this works, it might be clearer to either:
- Omit position and axisRole from the base and only set them in the role-specific configs
- Rename it to clearly indicate it's X-axis based
This is a minor clarity issue and doesn't affect functionality.
src/plugins/explore/public/application/utils/hooks/use_initialize_metrics_dataset.ts (2)
49-51: First connection used without validation.The code assumes
result.children[0]exists and is valid without checking its structure. While the outerif (result.children && result.children.length > 0)check ensures the array isn't empty, it doesn't validate the connection object itself.Consider adding validation:
if (result.children && result.children.length > 0) { const firstConnection = result.children[0]; + if (!firstConnection.id || !firstConnection.type) { + return; // or log/handle invalid connection + } const dataset = typeConfig.toDataset([datasetRoot, firstConnection]);
53-63: Magic boolean parameter lacks clarity.The third parameter
falsepassed tocacheDatasetat line 62 has no clear meaning without reading the function signature or documentation.Consider using a named constant or inline comment:
+ const SKIP_REFRESH = false; // or whatever the parameter means await datasetService.cacheDataset( dataset, { uiSettings: services.uiSettings, savedObjects: services.savedObjects, notifications: services.notifications, http: services.http, data: services.data, }, - false + SKIP_REFRESH );src/plugins/explore/public/application/pages/metrics/metrics_bottom_container/bottom_right_container/resizable_vis_control_and_tabs.tsx (1)
19-19: Avoid internal OUI import that may break on library updates.This file imports
PanelDirectionfrom@elastic/eui/src/components/resizable_container/types, which is an internal library path. This pattern doesn't appear elsewhere in the codebase and is fragile to library changes. Define the type locally instead.-import { PanelDirection } from '@elastic/eui/src/components/resizable_container/types'; +// Define locally to avoid internal OUI dependency +type PanelDirection = 'left' | 'right';src/plugins/explore/public/application/pages/metrics/metrics_bottom_container/bottom_right_container/bottom_right_container.tsx (1)
91-97: Unnecessary fragment wrapper around CanvasPanel.The empty fragment (
<>...</>) wrappingCanvasPanelis unnecessary and can be removed for cleaner JSX.🔎 Suggested simplification
) { return ( - <> - <CanvasPanel> - <ResizableVisControlAndTabs /> - </CanvasPanel> - </> + <CanvasPanel> + <ResizableVisControlAndTabs /> + </CanvasPanel> ); }src/plugins/explore/public/components/visualizations/utils/data_transformation.ts (2)
59-69: Transform pipeline accumulates intermediate results.The pipeline pushes each transformation result to
transformedDataarray, preserving the transformation history. This could be useful for debugging but increases memory usage. Consider if only the final result is needed.
130-204: Consider extracting shared grouping logic betweenaggregateandpivot.Both functions contain nearly identical logic for:
- Time unit inference (lines 142-146, 355-359)
- Time-based vs categorical grouping (lines 149-170, 362-383)
- Invalid date handling
- Result sorting (lines 199-201, 414-416)
This duplication increases maintenance burden. Consider extracting a shared
groupByFieldhelper.Also applies to: 337-419
src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts (2)
107-119: Consider adding a comment explaining the fallback behavior.This function returns
'value'for unknown/undefined axes, whereas the similargetAxisTypeinutils.tsreturns'unknown'for the default case. The difference is reasonable for ECharts (which needs a valid axis type), but a brief comment would clarify the intentional deviation.
266-268: Avoid reassigning function parameter.Reassigning
seriesFieldsmakes the code harder to follow. Use a separate variable for the resolved value.🔎 Proposed fix
- if (!Array.isArray(seriesFields)) { - seriesFields = seriesFields(source[0]); - } + const resolvedSeriesFields = Array.isArray(seriesFields) + ? seriesFields + : seriesFields(source[0]);Then use
resolvedSeriesFieldsin the rest of the function.
| feat: | ||
| - Introducing ECharts to discover visualization ([#11077](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11077)) | ||
|
|
||
| refactor: | ||
| - Refactor bar charts from Vega to ECharts ([#11077](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11077)) No newline at end of file |
There was a problem hiding this comment.
Critical inconsistency: Changelog references wrong PR and unrelated features.
This changelog fragment references PR #11077 and documents ECharts-related changes ("Introducing ECharts to discover visualization", "Refactor bar charts from Vega to ECharts"), but the PR objectives state this is PR #11094 focused on adding "Ask AI" capability with Claude 4 integration and screenshot capture. There is no mention of ECharts, Vega, or discovery visualization in the PR objectives.
Either:
- This changelog belongs to PR Discover vis echarts init #11077 and was incorrectly included in this review, or
- The PR objectives/description provided are incorrect
Please verify that the correct files are under review and that changelog entries match the actual scope of PR #11094.
🤖 Prompt for AI Agents
In changelogs/fragments/11077.yml around lines 1 to 5, the fragment references
PR #11077 and ECharts/Vega changes which do not match this review's PR #11094
(Ask AI / Claude 4 / screenshot capture); verify whether this file was
mistakenly included or the PR description is wrong, then either replace the
fragment with the correct changelog items for PR #11094 (accurate summary lines,
correct PR number/link, and brief bullet points for Ask AI, Claude 4
integration, and screenshot capture) or restore the original fragment for PR
#11077 and remove it from this review; also scan changelogs/fragments for other
mismatched files and update PR numbers/links to ensure consistency.
| feat: | ||
| - [Data] Support flat_object field type ([#11085](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11085)) No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the changelog fragment file exists and view its content
cat -n changelogs/fragments/11085.yml
# Also check if there's a changelog format guide
find . -name "*.md" -o -name "*.txt" | xargs grep -l "changelog" | head -5
# List all changelog fragments to understand the structure
ls -la changelogs/fragments/ | head -20
# Check a few other changelog fragments for comparison
fd -e yml . changelogs/fragments/ --max-results 5 --exec sh -c 'echo "=== File: {} ===" && head -3 {}'Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 1811
🏁 Script executed:
# Check the changelog README for format guidelines
cat changelogs/README.md
# Check git status for this file
git status changelogs/fragments/11085.yml 2>/dev/null || echo "File status check"
# Check git log for this file to see which PR originally added it
git log --oneline --all -- changelogs/fragments/11085.yml 2>/dev/null | head -5
# Verify YAML syntax by viewing a few other fragments for comparison
echo "=== Example: 10935.yml ===" && cat changelogs/fragments/10935.yml
echo "=== Example: 11023.yml ===" && cat changelogs/fragments/11023.yml
# Try to validate the YAML of the current file
python3 << 'EOF'
import yaml
try:
with open('changelogs/fragments/11085.yml', 'r') as f:
content = yaml.safe_load(f)
print("✓ YAML is valid")
print(f"Parsed content: {content}")
except yaml.YAMLError as e:
print(f"✗ YAML error: {e}")
except Exception as e:
print(f"✗ Error: {e}")
EOFRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 1156
🏁 Script executed:
# Check for hidden characters and file encoding issues
echo "=== Checking 11085.yml for hidden characters ==="
od -c changelogs/fragments/11085.yml
echo -e "\n=== Checking 11023.yml for comparison ==="
od -c changelogs/fragments/11023.yml | head -20
# Check file properties
echo -e "\n=== File properties ==="
file changelogs/fragments/11085.yml
file changelogs/fragments/11023.yml
# Check for trailing newline
echo -e "\n=== Trailing newline check ==="
tail -c 5 changelogs/fragments/11085.yml | od -c
tail -c 5 changelogs/fragments/11023.yml | od -cRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 505
🏁 Script executed:
# Use python to inspect file encoding and content
python3 << 'EOF'
import os
files_to_check = ['changelogs/fragments/11085.yml', 'changelogs/fragments/11023.yml']
for filepath in files_to_check:
print(f"\n=== Analyzing {filepath} ===")
try:
# Read as binary
with open(filepath, 'rb') as f:
content_bytes = f.read()
print(f"File size: {len(content_bytes)} bytes")
print(f"Raw bytes (hex): {content_bytes.hex()}")
print(f"Raw bytes (repr): {repr(content_bytes)}")
# Try to decode
try:
content_str = content_bytes.decode('utf-8')
print(f"UTF-8 decode: OK")
except:
print(f"UTF-8 decode: FAILED")
# Show line by line
with open(filepath, 'r', encoding='utf-8', errors='replace') as f:
lines = f.readlines()
print(f"Lines: {len(lines)}")
for i, line in enumerate(lines, 1):
print(f" Line {i} (repr): {repr(line)}")
except Exception as e:
print(f"Error: {e}")
EOF
# Try hexdump if available
echo -e "\n=== Attempting hexdump ==="
which hexdump && hexdump -C changelogs/fragments/11085.yml || echo "hexdump not available"Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 1617
🏁 Script executed:
# Check if other fragments have final newlines
python3 << 'EOF'
import os
fragment_dir = 'changelogs/fragments'
files = sorted([f for f in os.listdir(fragment_dir) if f.endswith('.yml')])[:10]
print("=== Checking final newlines in changelog fragments ===")
for filename in files:
filepath = os.path.join(fragment_dir, filename)
with open(filepath, 'rb') as f:
content = f.read()
has_final_newline = content.endswith(b'\n')
print(f"{filename:20} | Size: {len(content):3} | Final newline: {has_final_newline}")
# Now validate YAML for both files
print("\n=== YAML Validation ===")
import yaml
test_files = ['changelogs/fragments/11085.yml', 'changelogs/fragments/11023.yml']
for filepath in test_files:
try:
with open(filepath, 'r') as f:
yaml.safe_load(f)
print(f"{os.path.basename(filepath)}: ✓ VALID")
except yaml.YAMLError as e:
print(f"{os.path.basename(filepath)}: ✗ INVALID")
print(f" Error: {str(e).split('chr(10)')[0]}") # Truncate error for readability
EOFRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 958
🏁 Script executed:
# The issue is clear now - let's test if quoting fixes the YAML
python3 << 'EOF'
import yaml
# Test the current content
current_content = """feat:
- [Data] Support flat_object field type ([#11085](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11085))"""
# Test with quoted [Data]
quoted_content = """feat:
- "[Data] Support flat_object field type ([#11085](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11085))""""
print("=== Testing Current Content ===")
try:
yaml.safe_load(current_content)
print("✓ VALID")
except yaml.YAMLError as e:
print(f"✗ INVALID: {str(e).splitlines()[0]}")
print("\n=== Testing Quoted Content ===")
try:
result = yaml.safe_load(quoted_content)
print("✓ VALID")
print(f"Parsed: {result}")
except yaml.YAMLError as e:
print(f"✗ INVALID: {str(e).splitlines()[0]}")
# Now check if this file should actually be in the current PR
print("\n=== Checking PR Context ===")
import subprocess
result = subprocess.run(['git', 'diff', '--name-only', 'HEAD~5..HEAD'],
capture_output=True, text=True)
print("Files changed in recent commits:")
print(result.stdout if result.stdout else "No changes found")
EOFRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 434
🏁 Script executed:
# Fix the Python script syntax
python3 << 'EOF'
import yaml
# Test the current content
current_content = """feat:
- [Data] Support flat_object field type ([#11085](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11085))"""
# Test with quoted [Data]
quoted_content = """feat:
- "[Data] Support flat_object field type ([#11085](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11085))" """
print("=== Testing Current Content ===")
try:
yaml.safe_load(current_content)
print("✓ VALID")
except yaml.YAMLError as e:
print(f"✗ INVALID: {str(e).splitlines()[0]}")
print("\n=== Testing Quoted Content ===")
try:
result = yaml.safe_load(quoted_content)
print("✓ VALID")
print(f"Parsed: {result}")
except yaml.YAMLError as e:
print(f"✗ INVALID: {str(e).splitlines()[0]}")
# Test with properly formatted YAML (description as string value)
fixed_content = """feat:
- "[Data] Support flat_object field type ([#11085](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11085))"
"""
print("\n=== Testing Fixed Content (with quotes) ===")
try:
result = yaml.safe_load(fixed_content)
print("✓ VALID")
print(f"Parsed: {result}")
except yaml.YAMLError as e:
print(f"✗ INVALID: {str(e).splitlines()[0]}")
EOF
# Check git diff to understand context
echo -e "\n=== Git Context ==="
git log --oneline -10Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 617
Fix YAML syntax error in changelog fragment by quoting the string value.
The YAML in this file is invalid due to the markdown link syntax. Line 2 starts with [Data], which YAML parser interprets as mapping syntax rather than a string literal.
Quote the entire entry:
feat:
- "[Data] Support flat_object field type ([#11085](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11085))"Additionally, verify that this changelog fragment (11085.yml) belongs to the current PR. The filename suggests it documents PR #11085, not the current PR.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 2-2: syntax error: expected , but found ''
(syntax)
🤖 Prompt for AI Agents
In changelogs/fragments/11085.yml around lines 1 to 2, the changelog entry is
unquoted so the YAML parser treats the leading [Data] as mapping syntax; wrap
the entire list entry in double quotes so the line becomes a single YAML string,
and ensure the fragment actually belongs to this PR (filename suggests it
documents PR #11085) — if it does not belong, remove or move the fragment to the
correct PR.
| feat: | ||
| - Data processing for applying threshold colors ([#11106](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11106)) No newline at end of file |
There was a problem hiding this comment.
Critical: Changelog fragment references wrong PR and describes different functionality.
This changelog fragment has significant inconsistencies:
- Wrong PR number: The file references PR [Explore Vis]Data processing for applying threshold colors #11106, but the current PR is Add 'Ask AI' capability to visualizations #11094
- Wrong feature description: The changelog describes "Data processing for applying threshold colors", but this PR adds "Ask AI" capability to visualizations with screenshot capture
- Skip-Changelog label: The PR metadata indicates this PR is labeled "Skip-Changelog", suggesting no changelog entry should be added
Either this file was included by mistake, or the content needs to be corrected to match the actual PR.
🔎 Expected content (if changelog is needed)
If a changelog entry is appropriate for this PR, it should reference the correct PR number and describe the Ask AI feature:
feat:
-- Data processing for applying threshold colors ([#11106](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11106))
+- Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#11094](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11094))However, given the "Skip-Changelog" label, consider whether this file should be removed entirely from the PR.
📝 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.
| feat: | |
| - Data processing for applying threshold colors ([#11106](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11106)) | |
| feat: | |
| - Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#11094](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11094)) |
🤖 Prompt for AI Agents
In changelogs/fragments/11106.yml around lines 1 to 2, the fragment references
the wrong PR and content for this change; either remove the fragment entirely
because this PR is labeled "Skip-Changelog", or replace its contents to match
this PR (#11094) — update the PR number to 11094 and change the message to
succinctly describe the added "Ask AI" visualization capability with screenshot
capture (or another accurate one-line changelog), ensuring the file content and
PR metadata are consistent.
| }, | ||
| "devDependencies": { | ||
| "@elastic/eui": "npm:@opensearch-project/oui@1.21.0", | ||
| "@elastic/eui": "npm:@opensearch-project/oui@1.22.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify OUI version 1.22.1 exists and check for security advisories
# Check if the version exists on npm
npm view @opensearch-project/[email protected] version 2>/dev/null || echo "Version check failed or not available"
# Get recent versions to confirm 1.22.1 is published
echo "=== Recent OUI versions (1.21.x and 1.22.x) ==="
npm view @opensearch-project/oui versions --json 2>/dev/null | jq -r '.[]' | grep -E '^1\.(21|22)\.' | tail -10Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 159
🌐 Web query:
OpenSearch OUI 1.22.1 release notes breaking changes migration
💡 Result:
Do you mean OpenSearch Dashboards (formerly OUI) version 1.22.1, the "opensearch-ui" frontend package, or something else? I can search and fetch the 1.22.1 release notes and list breaking changes + migration steps — which exact project/package should I look up?
🌐 Web query:
@opensearch-project/oui 1.22.1 release notes GitHub
💡 Result:
I couldn't find a published GitHub release or tag named "1.22.1" for opensearch-project/oui. The repository and its CHANGELOG are here if you want me to look for a specific entry or copy the notes:
- Repository: opensearch-project/oui. [1]
- CHANGELOG.md in the repo. [2]
Would you like me to search the CHANGELOG for "1.22.1" or look for a release tag elsewhere (npm, tags page, or a specific commit)?
Sources:
[1] https://github.com/opensearch-project/oui
[2] https://github.com/opensearch-project/oui/blob/main/CHANGELOG.md
🌐 Web query:
site:github.com opensearch-project/oui releases 1.22.1
💡 Result:
I couldn't find a GitHub release or tag "1.22.1" for opensearch-project/oui. The repository page is here [1], and the npm package shows latest published version 1.21.0 [2].
Sources:
[1] https://github.com/opensearch-project/oui
[2] https://www.npmjs.com/package/@opensearch-project/oui
🌐 Web query:
@opensearch-project/oui npm registry all versions 1.22
💡 Result:
There is no @opensearch-project/oui v1.22 on the npm registry — the latest published version is 1.21.0. [1][2]
You can list all published versions yourself with:
npm view @opensearch-project/oui versions --json
Sources:
[1] npm package page for @opensearch-project/oui (shows version 1.21.0).
[2] GitHub repo for opensearch-project/oui.
🌐 Web query:
site:github.com/opensearch-project/oui CHANGELOG 1.22.1 1.22.0
💡 Result:
I couldn't find explicit 1.22.1 / 1.22.0 entries for opensearch-project/oui in the repo search results. Do you want me to:
- fetch and show the oui/CHANGELOG.md contents and search it for those versions, or
- search releases/tags for v1.22.1 and v1.22.0 and return their notes?
Which do you prefer?
🏁 Script executed:
cat packages/osd-ui-framework/package.json | head -50Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 1098
🏁 Script executed:
# Also check if there are any other references to OUI versions in the codebase
rg "@opensearch-project/oui|@elastic/eui" packages/osd-ui-framework/ -A 2Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 4068
Correct the OUI dependency version—1.22.1 does not exist on the npm registry.
The specified version @opensearch-project/[email protected] is not published on npm. The latest available version is 1.21.0. Update the dependency to a version that exists (either 1.21.0 or verify the intended version if 1.22.1 is planned for future release).
🤖 Prompt for AI Agents
In packages/osd-ui-framework/package.json at line 26, the @elastic/eui alias
points to a non-existent npm version "@opensearch-project/[email protected]"; update
this dependency to a published version (for example
"@opensearch-project/[email protected]") or the correct intended version, and run npm
install / yarn install to verify resolution.
| // Handle instant data for Prometheus queries - transform to hits format | ||
| if (data.meta?.instantData) { | ||
| const instantData = data.meta.instantData; | ||
| const instantHits = instantData.rows.map((row: Record<string, unknown>) => ({ | ||
| _index: data.name, | ||
| _source: row, | ||
| })); | ||
| (searchResponse as any).instantHits = { | ||
| hits: instantHits, | ||
| total: instantHits.length, | ||
| }; | ||
| (searchResponse as any).instantFieldSchema = instantData.schema; | ||
| } |
There was a problem hiding this comment.
Add defensive checks for instantData properties.
The code assumes instantData.rows and instantData.schema exist without validation. If instantData is present but malformed, calling .map() on undefined will throw a runtime error.
🔎 Proposed fix
// Handle instant data for Prometheus queries - transform to hits format
if (data.meta?.instantData) {
const instantData = data.meta.instantData;
- const instantHits = instantData.rows.map((row: Record<string, unknown>) => ({
- _index: data.name,
- _source: row,
- }));
- (searchResponse as any).instantHits = {
- hits: instantHits,
- total: instantHits.length,
- };
- (searchResponse as any).instantFieldSchema = instantData.schema;
+ if (instantData.rows && Array.isArray(instantData.rows)) {
+ const instantHits = instantData.rows.map((row: Record<string, unknown>) => ({
+ _index: data.name,
+ _source: row,
+ }));
+ (searchResponse as any).instantHits = {
+ hits: instantHits,
+ total: instantHits.length,
+ };
+ if (instantData.schema) {
+ (searchResponse as any).instantFieldSchema = instantData.schema;
+ }
+ }
}📝 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.
| // Handle instant data for Prometheus queries - transform to hits format | |
| if (data.meta?.instantData) { | |
| const instantData = data.meta.instantData; | |
| const instantHits = instantData.rows.map((row: Record<string, unknown>) => ({ | |
| _index: data.name, | |
| _source: row, | |
| })); | |
| (searchResponse as any).instantHits = { | |
| hits: instantHits, | |
| total: instantHits.length, | |
| }; | |
| (searchResponse as any).instantFieldSchema = instantData.schema; | |
| } | |
| // Handle instant data for Prometheus queries - transform to hits format | |
| if (data.meta?.instantData) { | |
| const instantData = data.meta.instantData; | |
| if (instantData.rows && Array.isArray(instantData.rows)) { | |
| const instantHits = instantData.rows.map((row: Record<string, unknown>) => ({ | |
| _index: data.name, | |
| _source: row, | |
| })); | |
| (searchResponse as any).instantHits = { | |
| hits: instantHits, | |
| total: instantHits.length, | |
| }; | |
| if (instantData.schema) { | |
| (searchResponse as any).instantFieldSchema = instantData.schema; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/plugins/data/common/data_frames/utils.ts around lines 130 to 142, the
code assumes data.meta.instantData.rows and .schema always exist; add defensive
checks so malformed instantData won't throw. Validate that data.meta.instantData
is an object, use Array.isArray(instantData.rows) before mapping (default to an
empty array if not), and only assign instantFieldSchema when instantData.schema
is a plain object (or default to {}). Ensure instantHits.total uses the
validated rows length and avoid calling .map on undefined by guarding or falling
back to [].
src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts
Outdated
Show resolved
Hide resolved
| echartsAxisConfig.axisLabel = { | ||
| show: !!axisStyle.labels.show, | ||
| // Prevent label overlap by showing all labels | ||
| interval: 0, | ||
| }; |
There was a problem hiding this comment.
Misleading comment: interval: 0 can cause label overlap.
The comment states "Prevent label overlap by showing all labels," but interval: 0 forces ECharts to display every label regardless of space, which can cause overlap. ECharts' default (interval: 'auto') actually prevents overlap by intelligently hiding labels.
If the intent is to ensure all labels are visible (accepting potential overlap), update the comment to reflect that. Otherwise, consider using 'auto' or a calculated interval.
🔎 Suggested comment fix
echartsAxisConfig.axisLabel = {
show: !!axisStyle.labels.show,
- // Prevent label overlap by showing all labels
+ // Force all labels to be visible (may cause overlap with many data points)
interval: 0,
};🤖 Prompt for AI Agents
In src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts
around lines 229 to 233, the inline comment claiming "Prevent label overlap by
showing all labels" is incorrect because interval: 0 forces every label to
render and can cause overlap; either change the code to use interval: 'auto' (or
compute a suitable interval) to let ECharts avoid overlap, or if you
intentionally want all labels shown despite overlap, update the comment to state
that interval: 0 forces all labels to display and may cause overlap; make the
chosen behavior explicit and keep comment and code consistent.
src/plugins/explore/public/components/visualizations/utils/echarts_spec.ts
Outdated
Show resolved
Hide resolved
| const hasSelectionMapping = Object.keys(visConfig?.axesMapping ?? {}).length !== 0; | ||
| if (hasSelectionMapping) { | ||
| if (!ExpressionRenderer) { | ||
| return null; | ||
| if (!spec.$schema) { | ||
| return <EchartsRender spec={spec} />; | ||
| } | ||
| const expression = toExpression(searchContext, spec); | ||
| return ( | ||
| <ExpressionRenderer | ||
| key={JSON.stringify(searchContext) + expression} | ||
| expression={expression} | ||
| <VegaRender | ||
| searchContext={searchContext} | ||
| onEvent={onExpressionEvent} | ||
| ExpressionRenderer={ExpressionRenderer} | ||
| onSelectTimeRange={onSelectTimeRange} | ||
| spec={spec} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Potential issue: spec may be undefined when accessed.
createVisSpec can return undefined (per relevant code snippet at lines 18-53 of create_vis_spec.ts). When hasSelectionMapping is true but spec is undefined, accessing spec.$schema will throw a TypeError.
Proposed fix
const hasSelectionMapping = Object.keys(visConfig?.axesMapping ?? {}).length !== 0;
- if (hasSelectionMapping) {
+ if (hasSelectionMapping && spec) {
if (!spec.$schema) {
return <EchartsRender spec={spec} />;
}📝 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.
| const hasSelectionMapping = Object.keys(visConfig?.axesMapping ?? {}).length !== 0; | |
| if (hasSelectionMapping) { | |
| if (!ExpressionRenderer) { | |
| return null; | |
| if (!spec.$schema) { | |
| return <EchartsRender spec={spec} />; | |
| } | |
| const expression = toExpression(searchContext, spec); | |
| return ( | |
| <ExpressionRenderer | |
| key={JSON.stringify(searchContext) + expression} | |
| expression={expression} | |
| <VegaRender | |
| searchContext={searchContext} | |
| onEvent={onExpressionEvent} | |
| ExpressionRenderer={ExpressionRenderer} | |
| onSelectTimeRange={onSelectTimeRange} | |
| spec={spec} | |
| /> | |
| ); | |
| const hasSelectionMapping = Object.keys(visConfig?.axesMapping ?? {}).length !== 0; | |
| if (hasSelectionMapping && spec) { | |
| if (!spec.$schema) { | |
| return <EchartsRender spec={spec} />; | |
| } | |
| return ( | |
| <VegaRender | |
| searchContext={searchContext} | |
| ExpressionRenderer={ExpressionRenderer} | |
| onSelectTimeRange={onSelectTimeRange} | |
| spec={spec} | |
| /> | |
| ); |
🤖 Prompt for AI Agents
In src/plugins/explore/public/components/visualizations/visualization_render.tsx
around lines 115 to 127, the code reads spec.$schema without guarding against
spec being undefined (createVisSpec can return undefined), which can throw;
update the conditional to check for spec existence before accessing $schema
(e.g., handle the spec === undefined case by returning a safe fallback or
EchartsRender with spec passed through) so the code never dereferences spec when
it's undefined.
| } catch (error) { | ||
| // Make sure to hide loading indicator on error | ||
| if (chatService.setCapturingImage) { | ||
| chatService.setCapturingImage(false); | ||
| } | ||
|
|
||
| this.core.notifications.toasts.addError(error, { | ||
| title: i18n.translate('visualizations.actions.askAI.error.title', { | ||
| defaultMessage: 'Failed to open chat', | ||
| }), | ||
| }); | ||
| } |
There was a problem hiding this comment.
**Error type may not be an Error instance.**The catch block at line 324 catches an error with unknown type, but addError on line 330 expects an Error instance. In JavaScript, you can throw anything—not just Error objects, but strings, numbers, objects, or even null. TypeScript's type system accounts for this reality, which is why TypeScript 4.4+ defaults to typing catch block errors as unknown. You can't assume it's an Error object until you verify it at runtime.
🔎 Suggested fix with type narrowing
} catch (error) {
// Make sure to hide loading indicator on error
if (chatService.setCapturingImage) {
chatService.setCapturingImage(false);
}
- this.core.notifications.toasts.addError(error, {
+ const errorObj = error instanceof Error ? error : new Error(String(error));
+ this.core.notifications.toasts.addError(errorObj, {
title: i18n.translate('visualizations.actions.askAI.error.title', {
defaultMessage: 'Failed to open chat',
}),
});
}🤖 Prompt for AI Agents
In src/plugins/visualizations/public/actions/ask_ai_action.tsx around lines 324
to 335, the catch block treats the caught value as an Error but TypeScript types
it as unknown; narrow the error at runtime before passing it to
core.notifications.toasts.addError (e.g. check if error is an instance of Error
and use it, otherwise create a new Error(String(error)) or wrap with new
Error(`Unexpected error: ${String(error)}`)); keep the existing logic that hides
the loading indicator (call chatService.setCapturingImage(false) if available)
and then pass the validated Error instance to addError with the same title
translation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
CHANGELOG.md (1)
7-10: Fix markdown indentation and replace TBD PR linkLine 9 has a leading space before the dash, which violates the
MD007markdown linting rule. Additionally, the PR link placeholder needs to be updated to the actual PR number.🔎 Proposed fix
### 📈 Features/Enhancements - - Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#TBD](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/TBD)) +- Add 'Ask AI' capability to visualizations with screenshot capture and Claude 4 integration ([#11094](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11094))src/plugins/visualizations/public/actions/ask_ai_action.tsx (2)
114-134: CSS selector injection vulnerability via visualization title remains unaddressed.The title is interpolated directly into CSS attribute selectors. If a title contains special characters like
"]or quotes, it could break the selector or cause unexpected matches.
324-335: Error type may not be an Error instance.The catch block catches
unknowntype, butaddErrorexpects anErrorinstance.src/plugins/chat/public/components/chat_window.test.tsx (1)
244-272: Orphan render call still present.Line 246 renders a
ChatWindowthat is never used. The test then creates a new ref and renders again on lines 261-262. This duplicates the component rendering unnecessarily. Per the past review comment, this was supposedly addressed but still appears in the code.🔎 Proposed fix
describe('loading message functionality', () => { it('should add loading message to timeline when sending a message', async () => { - renderWithContext(<ChatWindow onClose={jest.fn()} />); - // Mock the sendMessage to return a controllable observable const loadingObservable = { subscribe: jest.fn(() => { // Don't call next immediately to simulate loading state return { unsubscribe: jest.fn() }; }), }; mockChatService.sendMessage.mockResolvedValue({ observable: loadingObservable, userMessage: { id: 'user-1', content: 'test', role: 'user' }, }); const ref = React.createRef<ChatWindowInstance>(); renderWithContext(<ChatWindow ref={ref} onClose={jest.fn()} />);
🧹 Nitpick comments (7)
src/plugins/chat/public/components/chat_input.scss (1)
53-65: Use EUI color variable instead of hardcodedwhitefor theme consistency.Line 58 uses a hardcoded
whitevalue for the background, which may cause issues in dark mode or custom themes.🔎 Suggested fix
&__imageAttachment { position: relative; display: inline-block; margin-bottom: 8px; padding: 4px; - background: white; + background: $euiColorEmptyShade; border-radius: 8px; border: 1px solid $euiColorLightShade;packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (2)
515-526: Avoid duplicateBuffer.from()calls for logging.The
Buffer.from(base64Data, 'base64')is called twice—once for the image block (line 507) and again for logging (lines 518, 524). This unnecessarily decodes the same data twice.🔎 Suggested fix
// Add image block to content array + const imageBytes = Buffer.from(base64Data, 'base64'); const imageBlock = { image: { format, // png, jpeg, gif, webp source: { - bytes: Buffer.from(base64Data, 'base64'), // Convert base64 string to Buffer + bytes: imageBytes, // Convert base64 string to Buffer }, }, }; // Add image block to the beginning of content array content.unshift(imageBlock); this.logger.info('✅ Successfully added image block to message', { format, base64Length: base64Data.length, - bufferLength: Buffer.from(base64Data, 'base64').length, + bufferLength: imageBytes.length, totalContentBlocks: content.length, imageBlockStructure: { hasImage: true, hasFormat: !!format, hasSource: true, - hasBytesBuffer: Buffer.isBuffer(Buffer.from(base64Data, 'base64')), + hasBytesBuffer: Buffer.isBuffer(imageBytes), }, });
499-510: Add validation and normalization for image format to prevent Bedrock API errors.AWS Bedrock Converse API only supports png, jpeg, gif, and webp formats. The current code doesn't validate the format before sending to the API, which could cause runtime failures if invalid formats are extracted from the data URI. Additionally, 'jpg' is commonly used in data URIs but needs to be normalized to 'jpeg' for Bedrock compatibility.
🔎 Suggested fix
if (imageDataMatch) { const [, format, base64Data] = imageDataMatch; + // Validate and normalize format for Bedrock API + const normalizedFormat = format.toLowerCase() === 'jpg' ? 'jpeg' : format.toLowerCase(); + const supportedFormats = ['png', 'jpeg', 'gif', 'webp']; + if (!supportedFormats.includes(normalizedFormat)) { + this.logger.warn('Unsupported image format for Bedrock', { + original: format, + supported: supportedFormats, + }); + // Skip adding image block for unsupported format + } else { // Add image block to content array const imageBlock = { image: { - format, // png, jpeg, gif, webp + format: normalizedFormat, source: { bytes: Buffer.from(base64Data, 'base64'), }, }, }; // Add image block to the beginning of content array content.unshift(imageBlock); + }src/plugins/visualizations/public/actions/ask_ai_action.tsx (3)
23-24: Remove unused variablesstrategyUsedandselectorUsed.These variables are assigned throughout the function but never read. They appear to be debugging artifacts.
🔎 Suggested fix
Either remove the variables entirely or add debug logging that uses them:
try { let targetElement: HTMLElement | null = null; - let strategyUsed = ''; + // Debug logging can be enabled by uncommenting: + // let strategyUsed = ''; // Strategy 1: Look for embeddable by data-embeddable-id (most reliable) targetElement = document.querySelector( `[data-embeddable-id="${embeddable.id}"]` ) as HTMLElement; - if (targetElement) { - strategyUsed = 'data-embeddable-id'; - }Or add logging at the end before html2canvas:
+ console.debug(`[AskAI] Screenshot capture strategy: ${strategyUsed}, selector: ${selectorUsed}`); + // Use html2canvas to capture the visualization const html2canvas = await import('html2canvas');Also applies to: 156-170
276-277: Hardcoded delay before screenshot capture.The 100ms delay is a magic number. Consider extracting to a named constant and adding a comment explaining why this delay is necessary.
🔎 Suggested fix
+const CHAT_WINDOW_RENDER_DELAY_MS = 100; +const SCREENSHOT_RETRY_DELAY_MS = 200; +const MAX_SCREENSHOT_RETRIES = 5; // ... // Wait a bit for the chat window to be fully rendered and ref to be established - await new Promise((resolve) => setTimeout(resolve, 100)); + await new Promise((resolve) => setTimeout(resolve, CHAT_WINDOW_RENDER_DELAY_MS));
308-320: Silent retry with no feedback or exponential backoff.The retry mechanism silently retries up to 5 times without any logging or user feedback. Consider adding debug logging and exponential backoff for robustness.
🔎 Suggested fix
let retries = 0; const maxRetries = 5; const setImageWithRetry = async () => { try { chatService.setPendingImage!(screenshot); } catch (error) { if (retries < maxRetries) { retries++; + console.debug(`[AskAI] Retrying setPendingImage (attempt ${retries}/${maxRetries})`); - await new Promise((resolve) => setTimeout(resolve, 200)); + await new Promise((resolve) => setTimeout(resolve, 200 * retries)); // Exponential backoff await setImageWithRetry(); + } else { + console.warn('[AskAI] Failed to set pending image after max retries'); } } };src/plugins/chat/public/components/chat_window.tsx (1)
291-292: Remove extra blank lines.There are consecutive blank lines that could be cleaned up for consistency.
📜 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 (22)
CHANGELOG.mdpackage.jsonpackages/osd-agents/configuration/default_model.jsonpackages/osd-agents/src/agents/langgraph/react_graph_nodes.tssrc/core/public/chat/chat_service.tssrc/core/public/chat/types.tssrc/plugins/chat/common/types.tssrc/plugins/chat/public/components/chat_input.scsssrc/plugins/chat/public/components/chat_input.test.tsxsrc/plugins/chat/public/components/chat_input.tsxsrc/plugins/chat/public/components/chat_input_loading.test.tsxsrc/plugins/chat/public/components/chat_window.test.tsxsrc/plugins/chat/public/components/chat_window.tsxsrc/plugins/chat/public/components/chat_window_loading.test.tsxsrc/plugins/chat/public/components/message_row.scsssrc/plugins/chat/public/components/message_row.tsxsrc/plugins/chat/public/plugin.tssrc/plugins/chat/public/services/chat_service.tssrc/plugins/visualizations/public/actions/ask_ai_action.test.tsxsrc/plugins/visualizations/public/actions/ask_ai_action.tsxsrc/plugins/visualizations/public/actions/index.tssrc/plugins/visualizations/public/plugin.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/plugins/visualizations/public/actions/index.ts
- src/core/public/chat/chat_service.ts
- src/plugins/chat/public/components/message_row.scss
- src/plugins/visualizations/public/plugin.ts
- src/plugins/chat/public/components/chat_input.tsx
- src/core/public/chat/types.ts
- src/plugins/chat/public/components/chat_input.test.tsx
- src/plugins/visualizations/public/actions/ask_ai_action.test.tsx
- src/plugins/chat/common/types.ts
- src/plugins/chat/public/plugin.ts
- package.json
- packages/osd-agents/configuration/default_model.json
🧰 Additional context used
🧬 Code graph analysis (7)
packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (2)
packages/osd-pm/dist/index.js (1)
Buffer(32620-32620)packages/osd-agents/src/utils/logger.ts (1)
error(80-85)
src/plugins/chat/public/components/chat_window.test.tsx (2)
src/plugins/chat/public/contexts/chat_context.tsx (1)
ChatProvider(23-33)src/plugins/chat/public/components/chat_window.tsx (2)
ChatWindow(44-46)ChatWindowInstance(28-33)
src/plugins/chat/public/components/message_row.tsx (2)
src/core/public/chat/types.ts (1)
UserMessage(63-67)src/plugins/chat/common/types.ts (1)
UserMessage(16-16)
src/plugins/chat/public/components/chat_window.tsx (3)
src/plugins/chat/public/services/chat_service.ts (1)
setPendingImage(215-219)src/core/public/chat/types.ts (1)
UserMessage(63-67)src/plugins/chat/public/components/chat_input.tsx (1)
ChatInput(24-97)
src/plugins/visualizations/public/actions/ask_ai_action.tsx (2)
src/plugins/visualizations/public/actions/index.ts (3)
ACTION_ASK_AI(6-6)AskAIActionContext(6-6)AskAIAction(6-6)src/plugins/ui_actions/public/index.ts (1)
IncompatibleActionError(44-44)
src/plugins/chat/public/components/chat_input_loading.test.tsx (1)
src/plugins/chat/public/components/chat_input.tsx (1)
ChatInput(24-97)
src/plugins/chat/public/services/chat_service.ts (1)
src/core/public/chat/types.ts (2)
UserMessage(63-67)Message(83-88)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
9-9: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ 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 cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: WhiteSource Security Check
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: lighthouse
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Lint and validate
- GitHub Check: bundle-analyzer
🔇 Additional comments (18)
src/plugins/chat/public/components/chat_input.scss (1)
31-40: Multiple!importantdeclarations may indicate style override complexity.These overrides are likely necessary to neutralize EUI's default field styling when an image is attached. The approach is acceptable, but consider adding a brief comment explaining why these overrides are needed for future maintainers.
packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (1)
486-493: Logging includes imageData length, which is appropriate for debugging without exposing sensitive data.The logging captures metadata (length, block counts) rather than the actual image content, which is a good practice for observability.
src/plugins/chat/public/components/chat_input_loading.test.tsx (1)
11-98: Good test coverage for loading indicator and image attachment states.The tests comprehensively cover:
- Loading indicator visibility based on
isCapturingImage- Image attachment rendering with
pendingImage- Placeholder text changes when image is attached
- Send button enablement logic with images
The test structure is well-organized and aligns with the ChatInput component implementation.
src/plugins/chat/public/components/message_row.tsx (2)
34-36: Type assertion is properly guarded by role check.The cast to
UserMessageon line 35 is safe becausehasImageon line 36 only evaluatesuserMessage.imageDatawhenmessage.role === 'user', ensuring the type assertion is valid when accessed.
53-64: Image rendering integrates well with existing message structure.The image is displayed before the markdown content, which provides a natural reading order. The
allowFullScreenoption is a good UX choice for visualization screenshots.src/plugins/chat/public/components/chat_window_loading.test.tsx (2)
76-124: Good coverage of ref-based image capture API.The tests verify that
setCapturingImageandsetPendingImageare properly exposed via the ref and can be called without errors. This ensures the public API contract is maintained.
146-165: Default message behavior is well-tested.The test verifies that when only an image is provided (empty content), the default message "Can you analyze this visualization?" is sent. This matches the implementation in ChatWindow.
src/plugins/chat/public/components/chat_window.tsx (3)
115-143: Image-aware message sending logic is well-implemented.The flow correctly:
- Allows sending with just an image (no text required)
- Provides a sensible default message for image-only sends
- Clears the pending image after sending
- Includes imageData in the timeline for display
266-271:useImperativeHandledependency array is technically incomplete but safe.The dependency array only includes
handleNewChat, but the exposed methods also referencesetPendingImageandsetIsCapturingImage. However, since these areuseStatesetters, they have stable references and won't cause stale closures. The current implementation is functionally correct.
245-256: Good cleanup of image state on new chat.Clearing both
pendingImageandisCapturingImagewhen starting a new chat ensures no stale image state carries over.src/plugins/chat/public/services/chat_service.ts (3)
215-225: LGTM! Proper delegation pattern for image state management.The new
setPendingImageandsetCapturingImagemethods correctly delegate to theChatWindowInstanceref. The silent no-op whenchatWindowRef.currentis null provides graceful degradation when the window isn't available.
241-288: LGTM! Consistent imageData flow through both code paths.The
imageDataparameter is correctly passed through thesendMessageWithWindowmethod to both:
- The delegate path via
chatWindowRef.current.sendMessage(line 265)- The fallback path via direct
sendMessagecall (line 287)The
userMessageconstruction also includesimageData(line 272), maintaining consistency with theUserMessagetype interface.
367-383: LGTM! Clean signature extension for image support.The
sendMessagemethod signature is properly extended to accept optionalimageData, and the parameter is correctly included in theuserMessageobject.src/plugins/chat/public/components/chat_window.test.tsx (5)
44-52: LGTM! Proper mock setup for suggested actions service.The
mockSuggestedActionsServiceis correctly added and wired into theChatProvidercontext, aligning with the updatedChatProviderinterface that now requires thesuggestedActionsServiceprop.Also applies to: 70-75
101-119: LGTM! Good coverage for new ref methods.Tests properly verify that
setPendingImageandsetCapturingImageare exposed via the component ref, consistent with theChatWindowInstanceinterface.
149-242: LGTM! Comprehensive image capture test coverage.The test suite effectively covers:
- State management for
isCapturingImageandpendingImage- State clearing on new chat
- Image data flow through
sendMessage- Default message fallback when only image is provided (empty content)
The assertion on line 237 for the default message
'Can you analyze this visualization?'documents the expected UX behavior.
274-356: LGTM! Good loading state lifecycle coverage.The tests properly verify loading message behavior across different observable outcomes:
- Response received (
nextcallback)- Error handling (
errorcallback)- Stream completion (
completecallback)Using
setTimeoutwithin the mock observables to simulate async behavior is a reasonable approach for testing the loading state transitions.
358-399: LGTM! Persistence tests properly updated.The tests now use the required
onCloseprop and correctly typed persisted messages withas constassertions to satisfy the stricter role typing.
| // Use html2canvas to capture the visualization | ||
| const html2canvas = await import('html2canvas'); | ||
| const canvas = await html2canvas.default(visualizationElement, { | ||
| backgroundColor: '#ffffff', | ||
| scale: 1, | ||
| logging: false, | ||
| useCORS: true, | ||
| allowTaint: true, | ||
| height: visualizationElement.offsetHeight || 400, | ||
| width: visualizationElement.offsetWidth || 600, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file to understand the full context
head -200 src/plugins/visualizations/public/actions/ask_ai_action.tsx | tail -50Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 1547
🏁 Script executed:
# Check for cross-origin content usage in visualization files
rg -n "crossOrigin|CORS|origin|img|image" --type=ts --type=tsx -g '!**/node_modules/**' src/plugins/visualizations/public/actions/ -C 2Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 111
🏁 Script executed:
# Look for any error handling related to tainted canvas or export operations
rg -n "taint|canvas.*export|export.*canvas" --type=ts --type=tsx -g '!**/node_modules/**' src/plugins/visualizations/ -C 3Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 111
🏁 Script executed:
# Check the usage context of ask_ai_action - what does it do with the canvas?
rg -n "html2canvas\|toDataURL\|toBlob" --type=ts --type=tsx -g '!**/node_modules/**' src/plugins/visualizations/public/actions/ -A 5Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 111
🏁 Script executed:
# Search more broadly for cross-origin and CORS usage in the visualization and actions directories
rg -n "crossOrigin|external|http|src\s*=" src/plugins/visualizations/public/actions/ -C 2Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 66
🏁 Script executed:
# Look at the complete ask_ai_action.tsx file to understand the full context
wc -l src/plugins/visualizations/public/actions/ask_ai_action.tsxRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 147
🏁 Script executed:
# Check for any issues or comments about canvas taint in the codebase
rg -n "taint|toDataURL|toBlob" src/plugins/visualizations/ -C 3Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 677
🏁 Script executed:
# Check if there are visualizations that embed external images
rg -n "img\s|<img|Image|image.*src" src/plugins/visualizations/public/ --type typescript -C 2Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 118
🏁 Script executed:
# Look for error handling related to AI action canvas capture
rg -n "html2canvas|canvas.*error|error.*canvas" src/plugins/visualizations/public/actions/ -C 3Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 950
🏁 Script executed:
# Let's look at the full context of the function
sed -n '150,190p' src/plugins/visualizations/public/actions/ask_ai_action.tsxRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 1185
🏁 Script executed:
# Search for any test files related to ask_ai_action
find src/plugins/visualizations -name "*ask_ai*" -o -name "*test*ask*"Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 208
🏁 Script executed:
# Look for any plugins or code that might load external images in visualizations
rg -n "img|image|src=" src/plugins/visualizations/public/ -C 1 | head -100Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 10992
🏁 Script executed:
# Check html2canvas documentation and best practices in comments
rg -n "html2canvas" src/plugins/visualizations/ -C 5Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 1328
🏁 Script executed:
# Examine the test file to see if this scenario is tested
cat src/plugins/visualizations/public/actions/ask_ai_action.test.tsxRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 4847
🏁 Script executed:
# Look at the broader context of how the screenshot is used after being returned
sed -n '290,320p' src/plugins/visualizations/public/actions/ask_ai_action.tsxRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 1202
🏁 Script executed:
# Look at the broader context around the captureVisualization function to see the full try-catch
sed -n '140,195p' src/plugins/visualizations/public/actions/ask_ai_action.tsxRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 1674
🏁 Script executed:
# Check what types of elements are captured - look for visualization content that might have external images
rg -n "external\|cross-origin\|http" src/plugins/visualizations/public/actions/ -C 3Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 66
🏁 Script executed:
# Verify if there's any actual loading of cross-origin images in typical visualizations
rg -n "url\(|href|src" src/plugins/visualizations/public/vislib/ -C 2 | head -50Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 165
🏁 Script executed:
# Check what visualizations are available and if any might load external images
find src/plugins -type d -name "*visualization*" | head -20Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 494
🏁 Script executed:
# Look for any image loading in visualization plugins (e.g., maps, images, etc.)
rg -n "fetch|import.*image|<img" src/plugins/visualizations/ --type ts -C 2 | head -60Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 6820
🏁 Script executed:
# Check if there are any documented issues or TODOs about this
rg -n "TODO|FIXME|BUG|allowTaint|taint" src/plugins/visualizations/public/actions/ -C 2Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 580
🏁 Script executed:
# Look at the html2canvas library documentation and how it's configured elsewhere
rg -n "html2canvas" src/plugins/ -B 3 -A 5Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 1123
🏁 Script executed:
# Check git history or comments to understand why allowTaint was added
rg -n "allowTaint" src/plugins/ -B 5 -A 2Repository: opensearch-project/OpenSearch-Dashboards
Length of output: 891
🏁 Script executed:
# Look at the function signature and full context to see error handling
sed -n '135,195p' src/plugins/visualizations/public/actions/ask_ai_action.tsxRepository: opensearch-project/OpenSearch-Dashboards
Length of output: 1727
Consider removing allowTaint: true or add explicit taint detection.
The combination of useCORS: true and allowTaint: true creates a logic conflict: if CORS-protected resources fail to load, allowTaint: true allows the canvas to be populated with cross-origin content, but canvas.toDataURL() will then fail on a tainted canvas. The resulting error is silently caught and returns null, but the root cause isn't surfaced. Either remove allowTaint: true to strictly rely on CORS (recommended), or detect canvas taint explicitly before calling toDataURL() to provide clearer feedback to users when cross-origin content can't be captured.
🤖 Prompt for AI Agents
In src/plugins/visualizations/public/actions/ask_ai_action.tsx around lines 173
to 183, the html2canvas call sets useCORS: true and allowTaint: true which can
produce a tainted canvas that later makes canvas.toDataURL() fail silently;
remove allowTaint: true to force strict CORS handling (recommended) or, if you
must keep allowTaint, add explicit taint detection after rendering and before
calling toDataURL() and surface a clear error to the user when the canvas is
tainted so the root cause (cross-origin content) is reported instead of
returning null.
9 participants
Description
This PR adds an "Ask AI" capability to visualizations in OpenSearch Dashboards. Users can now right-click on any visualization and select "Ask AI" to capture a screenshot and ask questions about the visualization using Claude 4 Sonnet with vision capabilities.
Key Features:
Issues Resolved
Screenshot
Testing the changes
Manual Testing Steps:
Automated Testing:
yarn test:jest --testPathPattern="(ask_ai_action\.test\.tsx|chat_input_loading\.test\.tsx|chat_window_loading\.test\.tsx)"Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
New Features
Tests
Style
Dependencies
Chores
✏️ Tip: You can customize this high-level summary in your review settings.