chore(compass-assistant): split drawer and provider usage for connections context COMPASS-9603 #7199
Conversation
…ss-ai-plugin-refactor
There was a problem hiding this comment.
Pull Request Overview
Refactors the compass-assistant component architecture to separate the drawer implementation from the provider, enabling access to connection context from within the drawer component.
Key changes:
- Split the
CompassAssistantDrawercomponent out from the provider for better flexibility - Updated component interfaces to use
Chatobjects directly instead of message arrays and callbacks - Added the drawer component to both desktop and web workspace configurations
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/compass/src/app/components/workspace.tsx | Adds CompassAssistantDrawer component to desktop workspace |
| packages/compass-web/src/entrypoint.tsx | Adds CompassAssistantDrawer component to web workspace |
| packages/compass-assistant/src/index.tsx | Updates exports to include the new drawer component |
| packages/compass-assistant/src/compass-assistant-drawer.tsx | New drawer component that wraps AssistantChat in a DrawerSection |
| packages/compass-assistant/src/assistant-provider.tsx | Refactored to provide context instead of rendering drawer directly |
| packages/compass-assistant/src/assistant-chat.tsx | Updated to accept Chat object and handle messaging internally |
| packages/compass-assistant/src/assistant-chat.spec.tsx | Updated tests to work with new Chat-based interface |
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.
| chat, | ||
| isEnabled: enableAIAssistant, | ||
| }); | ||
| assistantContext.current = { |
There was a problem hiding this comment.
actually not sure this ref is doing much here
There was a problem hiding this comment.
Not in the way it's currently defined, yeah. I'd suggest to leave only chat instance in this context to make sure that a stable instance is being passed here and instead of passing isEnabled with it, read it from preferences hook in the CompassAssistantDrawer component (or just using null as a value that indicates that chat is not enabled, I don't think we need to be too strict with differentiating)
There was a problem hiding this comment.
my worry with preferences was the drawer ending up rendering before the provider ends up passing context but I guess it'd be top-down so it'd be fine.
gagik
force-pushed
the
gagik/compass-ai-plugin-refactor
branch
from
b28047a to
f65bcf6
Compare
| <CreateNamespacePlugin></CreateNamespacePlugin> | ||
| <DropNamespacePlugin></DropNamespacePlugin> | ||
| <RenameCollectionPlugin></RenameCollectionPlugin> | ||
| <CompassAssistantDrawer /> |
There was a problem hiding this comment.
I can add Plugin and turn this into an actual plugin if needed. This seemed like the most fitting place
There was a problem hiding this comment.
Not really required if we don't have a reason for it be a plugin, so all good here from my perspective
| > = ({ chat, children }) => { | ||
| const enableAIAssistant = usePreference('enableAIAssistant'); | ||
|
|
||
| const { messages, sendMessage } = useChat({ |
There was a problem hiding this comment.
the chat.sendMessage is basically the same as the one returned by the hook. The hook basically just subscribes to the chat for re-rendering based on chat so we can just use it as our context object
|
Going to adjust the structure a bit more to make it cleaner |
| const assistantContext = useRef<AssistantContextType>({ | ||
| chat, | ||
| }); | ||
| assistantContext.current = { | ||
| chat, | ||
| }; |
There was a problem hiding this comment.
Same issue as before: you're passing .current to provider and it's re-created every render due do this assignment. You can just directly pass chat from props to provider and completely drop this ref logic (chat being "stable" is guaranteed by us creating it once in "activate" method)
Uh oh!
There was an error while loading. Please reload this page.