chore(compass-assistant): hide the assistant buttons when AI features are disabled COMPASS-9760 #7262
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements proper hiding of assistant-related UI buttons when AI features are disabled, expanding the existing behavior for NLQ to other assistant features.
- Refactors assistant actions to return empty object when AI features are disabled
- Updates connection error toast to conditionally show debug button based on assistant availability
- Simplifies explain plan modal to remove redundant assistant enablement check
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-assistant/src/compass-assistant-provider.tsx | Refactors useAssistantActions to check AI feature enablement and return empty object when disabled |
| packages/compass-assistant/src/compass-assistant-provider.spec.tsx | Adds comprehensive tests for useAssistantActions behavior with different AI feature states |
| packages/compass-assistant/src/compass-assistant-drawer.tsx | Updates clearChat call to handle optional function |
| packages/compass-connections/src/stores/connections-store-redux.ts | Simplifies connection error handling by checking assistant availability directly |
| packages/compass-connections/src/components/connection-status-notifications.tsx | Updates toast interface to use optional callbacks instead of boolean flags |
| packages/compass-explain-plan/src/components/explain-plan-modal.tsx | Removes redundant isAssistantEnabled check |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/compass-connections/src/components/connection-status-notifications.tsx
Outdated
Show resolved
Hide resolved
| showDebugButton: boolean; | ||
| onReviewClick: () => void; | ||
| onDebugClick: () => void; | ||
| onReviewClick?: () => void; |
There was a problem hiding this comment.
personal preference but I think since showing and the handler are inherently tied it's good to combine the two. It's a pattern in LG too so works out pretty well usually
| @@ -37,22 +40,22 @@ export const AssistantContext = createContext<AssistantContextType | null>( | |||
| ); | |||
|
|
|||
| type AssistantActionsContextType = { | |||
| interpretExplainPlan: ({ | |||
| interpretExplainPlan?: ({ | |||
There was a problem hiding this comment.
I was thinking about making these optional and using the existence of the callbacks to be able to tell if the thing should be enabled or not, but was worried that it kinda disabled some type checking benefits - you can't tell between the feature being enabled/disabled vs a mock/stub just not containing it, for example.
But I was probably overthinking it.
Lower down like in the connection error entry point it indeed makes sense to have just the optional callback rather than a boolean and the optional callback. This high up.. I don't know. It is probably fine.
I don't have strong feelings about it.
There was a problem hiding this comment.
the alternative tends to make us be attentive in also including the isEnabled returned from the hook in our conditions for showing the button which can be easy to oversee so I find this to be more type checked for this purpose. Open to revisiting it later if we run into problems.
| @@ -160,7 +155,7 @@ export const CompassAssistantProvider = registerCompassPlugin( | |||
| transport: new DocsProviderTransport({ | |||
| baseUrl: atlasService.assistantApiEndpoint(), | |||
| }), | |||
| onError: (err) => { | |||
| onError: (err: Error) => { | |||
There was a problem hiding this comment.
I swear this gets either added or removed every other PR 😆
There was a problem hiding this comment.
I am rebasing so much everything feels like a deja-vu 😆
Will follow up later when we do the e2e itself
Expands it to match NLQ.