-
Notifications
You must be signed in to change notification settings - Fork 228
chore(compass-assistant): add compass AI assistant with placeholders COMPASS-9603 #7189
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
onSendMessage?: (message: string) => void; | ||
} | ||
|
||
export const AsisstantChat: React.FunctionComponent<AsisstantChatProps> = ({ |
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.
AI-generated placeholders for now...
const CompassAssistant = CompassAssistantProvider.withMockServices({}); | ||
|
||
it('renders a Plugin', function () { | ||
render(<CompassAssistant></CompassAssistant>); |
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.
not sure what kind of tests, if any, are worth the effort at the moment, especially with the placeholders
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 mean, visuals are visuals, but we still expect, for example, an input field that you should be able to input text in, submit it, see it in the output, get the response, etc. If you follow some of these best practices from testing library docs, I think you can already write some pretty good tests that wouldn't need to change much when we swap the placeholder UI
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.
But also you know, take this advice with a grain of salt 🙂 If it really doesn't make sense to you still, we can omit until we have more code here, just seems like a lot of it is already functional stuff
4fc9f43
to
989329b
Compare
bc3ac19
to
9fc210d
Compare
}, | ||
"devDependencies": { | ||
"react-dom": "^17.0.2", | ||
"throttleit": "^2.1.0", |
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.
depcheck forced me to move this throttleit to dev dependencies... not sure how right that is, could add that to depcheckrc ignores
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.
Hmmm, it shouldn't if the dependency is used in the source code, can you share the error you're seeing?
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.
Maybe @
in the filepath trips up the check, maybe worth removing it from the name and trying again
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.
didn't help, very weird..
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.
Tried it locally, it's not depcheck, it's our check-peer-deps script, seems like something is tripping up the module resolution logic there so it doesn't pick up the ./chat.react
file for parsing (and so misses the throttleit import). Looking
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 a new AI Assistant feature to Compass as a feature-flagged plugin. It provides a chat interface for MongoDB-related queries, using placeholder components while Leafygreen UI updates are in development.
- Introduces the
@mongodb-js/compass-assistant
package with AI chat functionality - Adds feature flag
enableAIAssistant
for controlled rollout - Integrates the assistant provider into both desktop and web Compass applications
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
packages/compass/src/app/utils/csp.ts | Adds CSP entries for MongoDB knowledge base endpoints |
packages/compass/src/app/components/home.tsx | Wraps application with CompassAssistantProvider |
packages/compass-web/src/entrypoint.tsx | Integrates assistant provider in web version |
packages/compass-web/package.json | Adds compass-assistant dependency |
packages/compass-preferences-model/src/feature-flags.ts | Adds enableAIAssistant feature flag |
packages/compass-assistant/ | New package with chat components and AI SDK integration |
0c73abf
to
b18904a
Compare
b18904a
to
95ddec6
Compare
const handleMessageSend = useCallback( | ||
(messageBody: string) => { | ||
/** Telemetry, etc. */ | ||
void sendMessage({ text: messageBody }); |
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.
Wondering if we should be swallowing this potential error. Should we not at least log 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.
we'd probably want to use onError
on the useChat hook. I'd say this should be followed up with more proper error handling in general, I'll create a ticket
return useContext(AssistantActionsContext); | ||
} | ||
|
||
export const AssistantProvider: React.FunctionComponent< |
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 super nitpicky, I don't know why this bothers me because, sure the top-level thing it returns IS a provider, but it is also kinda the drawer section. Just wondering if there's a better name.
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.
(but also feel free to just ignore me)
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's a provider since we're eventually wrapping other elements in it. otherwise it'd seem like Compass is wrapped inside a drawer section
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.
No strong opinion, but I think you can just call the public export CompassAssistant without mentioning either drawer or provider 🙂
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.
To me it is almost more about the filename than the export. But really this is SUPER nitpicky and I can live with any of this :)
messages: convertToModelMessages(messages), | ||
abortSignal: abortSignal, | ||
headers: { | ||
origin: 'https://knowledge.staging.corp.mongodb.com', |
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.
Is the .staging
part indicating that this is something temporary? Should this origin eventually be something that indicates it's coming from Compass?
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 new chatbot is only on staging so far and hasn't been employed to production. I imagine we'll have to change it over before this feature lands.
import { createOpenAI } from '@ai-sdk/openai'; | ||
|
||
const openai = createOpenAI({ | ||
baseURL: 'https://knowledge.staging.corp.mongodb.com/api/v1', |
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.
Should this be overridable through e.g. an environment variable?
messages: convertToModelMessages(messages), | ||
abortSignal: abortSignal, | ||
headers: { | ||
origin: 'https://knowledge.staging.corp.mongodb.com', |
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.
Browser won't propagate origin header to the actual request and won't allow you to override the one it sets, this is not really doing anything
import { createOpenAI } from '@ai-sdk/openai'; | ||
|
||
const openai = createOpenAI({ | ||
baseURL: 'https://knowledge.staging.corp.mongodb.com/api/v1', |
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 might not be using atlas service directly for fetching, but I think we should still at least have this url as part of AtlasServiceConfig
(and so similarly dynamically accessed from 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.
this has a dependency on the preferences object right? could I dynamically access it inside registerCompassPlugin
activate
?
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.
ah, I suppose with
import { atlasServiceLocator } from '@mongodb-js/atlas-service/provider';
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.
You might need to reshuffle components a bit to make sure that atlas service is above this one, but otherwise yeah: use a locator, get a config value from it in activate, pass it to the transport (a bit of an advanced pattern: if you want to create a "service" for the transport, you can package all this logic inside of it, you can look at some usage of createServiceProvider
method for something like this, but also no need to complicate it too much, just an idea)
{ | ||
transport: () => docsProviderTransport, | ||
} |
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 needs to be a specially created service locator fuction if you want to inject it, but also doesn't look like you're actually using it? Not a bad idea though, while we will be using the same instance in all runtimes, it will still allow you to override it in tests for example
2facc35
to
354afd6
Compare
? atlasService.assistantApiEndpoint() | ||
: 'https://knowledge.staging.corp.mongodb.com/api/v1'; | ||
const chat = new Chat({ | ||
transport: new DocsProviderTransport({ |
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.
@gribnoysup moved the transport inside the activate, I was originally exposing it for mocking (which I didn't end up doing) but I don't need to mock at such a low-level anyhow.. Not sure if the defaulting to staging bit for now is fine, just figured it'd be good to make this simple with the feature flag.
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 think it's fine to default to it if we have to, but I'd rather we just do this in the config (mentioned this in #7189 (comment))
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.
Looks good! Left a few small comments
Oh, and some of the failures are legit, so please take a look! |
|
||
return ( | ||
<AssistantActionsContext.Provider value={contextActions.current}> | ||
<DrawerSection |
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 what I was talking about in slack: Just move this somewhere lower down in the render tree, keeping <AssistantActionsContext.Provider where it is.
Then <AssistantChat should easily be able to use the connections and figure out if it should display some warning banner, but connections and every other component can still use the assistant context.
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.
(Obviously not necessary for this PR, though)
This adds a new Compass plugin for the AI Assistant. It is feature flagged and currently uses placeholders in place of Leafygreen chat components to allow us to test out the assistant right away and iterate on non-aesthetic features while progress is being made towards the Leafygreen components update.
This includes a vendored subset of
@ai-sdk/react
hooks