feat(compass-assistant): add a system prompt to precede user prompts with the basic UI context COMPASS-10140 #7629
Conversation
lerouxb
force-pushed
the
assistant-context
branch
from
6afcbc8 to
5ca54ad
Compare
| currentActiveConnection: null, | ||
| currentWorkspaceCollectionInfo: null, | ||
| }, | ||
| expected: 'The user does not have any tabs open.', |
There was a problem hiding this comment.
These expected lines are going to be extremely brittle. ie. all of them will have to be changed whenever we change the prompt. I'm trying to figure out a way to still have a reasonable level of testing but have something that will at least survive minor edits. 🤔
There was a problem hiding this comment.
The obvious modern way to do this to have these as eval cases and then use an LLM to score it via something like braintrust. But I do still like having unit tests.
There was a problem hiding this comment.
Pull request overview
This PR adds a system prompt feature that provides basic UI context (current workspace, active connections, and collection information) to the assistant before user prompts. The context is dynamically synced from various parts of the application and automatically included when sending messages to the assistant.
Key changes:
- Introduces a global state management system for assistant context via React Context
- Adds hooks to sync workspace and connection state with the assistant
- Implements context prompt generation that describes the user's current UI state
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-assistant/src/assistant-global-state.tsx | New file implementing global state management for assistant context |
| packages/compass-assistant/src/prompts.ts | Adds buildContextPrompt function to generate system messages from UI state |
| packages/compass-assistant/src/prompts.spec.ts | Comprehensive test coverage for context prompt generation |
| packages/compass-assistant/src/compass-assistant-provider.tsx | Integrates global state and modifies message sending to include context |
| packages/compass-assistant/src/compass-assistant-provider.spec.tsx | Adds test for context message inclusion |
| packages/compass-assistant/src/components/assistant-chat.tsx | Filters out system context messages from UI display |
| packages/compass-workspaces/src/components/index.tsx | Syncs workspace state with assistant global state |
| packages/compass-connections/src/index.tsx | Syncs active connections with assistant global state |
| packages/compass-assistant/src/preset-messages.ts | Changes NON_GENUINE_WARNING_MESSAGE role to 'system' |
| packages/compass-assistant/src/index.tsx | Exports useSyncAssistantGlobalState hook |
| packages/compass-assistant/package.json | Adds workspace-info dependency |
| packages/compass-workspaces/package.json | Adds compass-assistant dependency |
| useEffect(() => { | ||
| setState((prevState) => { | ||
| return { | ||
| ...prevState, | ||
| [stateKey]: newState, | ||
| }; | ||
| }); | ||
| }, [newState, setState, stateKey]); |
There was a problem hiding this comment.
The useEffect dependencies include newState which will trigger updates on every render if the value is a non-primitive (object/array). This can cause unnecessary re-renders. Consider using a deep comparison or memoizing the value before passing it to this hook.
lerouxb
force-pushed
the
assistant-context
branch
from
74dba10 to
aafc486
Compare
| isPermanent: true, | ||
| }, | ||
| role: 'assistant', | ||
| role: 'system', |
| activeTab?.type === 'Collection' | ||
| ? state.collectionInfo[activeTab.namespace] | ||
| ? state.collectionInfo[ | ||
| `${activeTab.connectionId}.${activeTab.namespace}` |
There was a problem hiding this comment.
drive-by. This has been broken for some time.
| ); | ||
| }; | ||
|
|
||
| export function useSyncAssistantGlobalState<T extends keyof GlobalState>( |
There was a problem hiding this comment.
Not sure how it should look like exactly, but you might want some sort of cleanup logic when the hook gets "unmounted" (so maybe not part of the same effect that updates the state, but an extra effect that sets stateKey back to null in cleanup?)
There was a problem hiding this comment.
Actually no that's dodgy. I can't control the order in which this unmounts so it clears each other sometimes 🤔
There was a problem hiding this comment.
OK I have changed this to something a lot more specific which is maybe not the best / most ideal "one size fits all" solution, but it seems to work for now and we can always revisit it. This is likely going to be hot code for a while anyway.
gribnoysup
left a comment
gribnoysup
left a comment
There was a problem hiding this comment.
I'd suggest to move some state subscriptions to different places, but it's not a blocker, overall lgtm
| activeCollectionMetadata: Pick< | ||
| CollectionMetadata, | ||
| 'isTimeSeries' | 'sourceName' | ||
| // TODO: isClustered, isFLE, isSearchIndexesSupported, isDataLake, isAtlas, serverVersion |
There was a problem hiding this comment.
I'll file a follow-up to pad out more context. We have all this metadata available on CollectionMetadata now and might as well use it. Then I'll augment this TODO line with the ticket number before I merge.
Building on top of main...poc-assistant-global-state
To get a sense of the system context prompts going out at the moment, have a look at prompts.spec.ts which tests a bunch of examples. We can obviously keep iterating on these now or in future.