-
Notifications
You must be signed in to change notification settings - Fork 245
chore(compass-assistant): add explain plan entry point COMPASS-9606 #7208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a0b2adf to
58624e8
Compare
package-lock.json
Outdated
| "mocha": "^10.2.0" | ||
| } | ||
| }, | ||
| "packages/bson-transpilers/node_modules/@typescript-eslint/eslint-plugin": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is weird... I tried checking out main and re-running from a clean slate and this still happens... is main package-lock in a bad state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be, but do double-check which version of npm you're running. I've seen it behave completely differently between versions recently giving me exactly this kind of behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone recently merged a pretty stale PR and this got in main, it's not great that these were duplicated, but also is not an issue really, this is a harmless cleanup, we're running checks in CI to make sure that package-lock in a good shape and doesn't have issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds an AI Assistant integration to the Explain Plan modal, allowing users to interpret explain plan output through natural language. The feature includes a new "Interpret for me" button that opens the AI Assistant drawer and sends the explain plan data for interpretation.
- Adds AI Assistant integration to the Explain Plan modal with conditional rendering based on preferences
- Implements explain plan interpretation functionality that converts JSON data to natural language
- Updates type definitions to support custom display text for AI Assistant messages
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-explain-plan/src/components/explain-plan-modal.tsx | Adds "Interpret for me" button and AI Assistant integration to the modal header |
| packages/compass-explain-plan/src/components/explain-plan-modal.spec.tsx | Adds tests for AI Assistant button visibility based on preferences |
| packages/compass-explain-plan/package.json | Adds compass-assistant dependency |
| packages/compass-assistant/src/prompts.ts | Creates explain plan prompt template for AI interpretation |
| packages/compass-assistant/src/index.tsx | Exports useAssistantActions hook |
| packages/compass-assistant/src/compass-assistant-provider.tsx | Implements interpretExplainPlan action and extends message type with metadata |
| packages/compass-assistant/test/utils.tsx | Updates mock chat utility to use new AssistantMessage type |
| packages/compass-assistant/src/compass-assistant-provider.spec.tsx | Updates tests to use new AssistantMessage type |
| packages/compass-assistant/src/assistant-chat.tsx | Adds support for displaying custom text via message metadata |
| packages/compass-assistant/src/assistant-chat.spec.tsx | Adds tests for custom display text functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| ?.filter((part) => part.type === 'text') | ||
| .map((part) => part.text) | ||
| .join('') || | ||
| ''} |
Copilot
AI
Aug 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The display logic uses a complex ternary expression that could be simplified for better readability. Consider extracting this into a helper function or variable to make the intent clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would do this, but it is indeed nitpicky.
| interpretExplainPlan: (explainPlan: string) => { | ||
| openDrawer(ASSISTANT_DRAWER_ID); | ||
| const { prompt, displayText } = buildExplainPlanPrompt(explainPlan); | ||
| void chat.sendMessage( |
Copilot
AI
Aug 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using 'void' to ignore the promise is acceptable here, but consider adding error handling for the sendMessage call to provide better user feedback if the operation fails.
| void chat.sendMessage( | |
| chat.sendMessage( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be followed up later and handled with onError on useChat hook
| Explain output: | ||
| ${explainPlan}`, | ||
| displayText: | ||
| 'Given this MongoDB explain plan, provide a concise human readable explanation that explains the query execution plan and highlights aspects of the plan that might impact query performance.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if for displaying purposes it's worth including something relevant to specific explain that was run, like a namespace or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could if we get designs to use them but from my understanding for v1 we're just going to hide i.e. the explain plan and let the response include relevant enough context.
Ah, it could also be in the text, right, good idea. I will ask this to Misba and add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explain might be very verbose to be converted to text, but I think even just adding a namespace to display text will go a long way here. I was playing around with this feature across multiple tabs and at some point it was not easy to navigate. To be very fair though this usage pattern is probably a corner case that doesn't matter that much 🙂
| const isAiAssistantEnabled = usePreference('enableAIAssistant'); | ||
| const { interpretExplainPlan } = useAssistantActions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a silly idea, but I wonder if we will almost always have these two together, maybe worth "re-exporting" the enabled prop as part of the useAssistant hook return value
| <Button | ||
| size="small" | ||
| variant="default" | ||
| leftGlyph={ | ||
| // TODO(COMPASS-9384): Will be replaced with Sparkle gradient icon once Leafygreen components are updated. | ||
| <Icon glyph="Sparkle" style={{ color: palette.green.dark1 }} /> | ||
| } | ||
| data-testid="interpret-for-me-button" | ||
| onClick={() => { | ||
| onModalClose(); | ||
| interpretExplainPlan(JSON.stringify(explainPlan)); | ||
| }} | ||
| > | ||
| Interpret for me | ||
| </Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This button should be disabled / hidden while we're still running the explain, otherwise you're going to be explaining no plan
87fd0ae to
0e82ba8
Compare
| }) => { | ||
| return { | ||
| prompt: `Given the MongoDB explain plan output below, provide a concise human readable explanation that explains the query execution plan and highlights aspects of the plan that might impact query performance. Respond with as much concision and clarity as possible. | ||
| If a clear optimization should be made, please suggest the optimization and describe how it can be accomplished in MongoDB Compass. Do not advise users to create indexes without weighing the pros and cons. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm so skeptical about this 😆
packages/compass-explain-plan/src/components/explain-plan-modal.spec.tsx
Show resolved
Hide resolved
|
Tested it locally and it works great! |
Adds the explain plan entry point.

