-
Notifications
You must be signed in to change notification settings - Fork 230
chore(compass-assistant): hide the assistant buttons when AI features are disabled COMPASS-9760 #7262
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
chore(compass-assistant): hide the assistant buttons when AI features are disabled COMPASS-9760 #7262
Changes from 4 commits
8d30961
9780de4
c5a8817
34a4cb0
8f7f026
7ce5a2e
62c9b64
70e9e73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,10 @@ import { | |
type EntryPointMessage, | ||
type ProactiveInsightsContext, | ||
} from './prompts'; | ||
import { usePreference } from 'compass-preferences-model/provider'; | ||
import { | ||
useIsAIFeatureEnabled, | ||
usePreference, | ||
} from 'compass-preferences-model/provider'; | ||
import { createLoggerLocator } from '@mongodb-js/compass-logging/provider'; | ||
import type { ConnectionInfo } from '@mongodb-js/connection-info'; | ||
import { useTelemetry } from '@mongodb-js/compass-telemetry/provider'; | ||
|
@@ -37,22 +40,22 @@ export const AssistantContext = createContext<AssistantContextType | null>( | |
); | ||
|
||
type AssistantActionsContextType = { | ||
interpretExplainPlan: ({ | ||
interpretExplainPlan?: ({ | ||
namespace, | ||
explainPlan, | ||
}: { | ||
namespace: string; | ||
explainPlan: string; | ||
}) => void; | ||
interpretConnectionError: ({ | ||
interpretConnectionError?: ({ | ||
connectionInfo, | ||
error, | ||
}: { | ||
connectionInfo: ConnectionInfo; | ||
error: Error; | ||
}) => void; | ||
clearChat: () => void; | ||
tellMoreAboutInsight: (context: ProactiveInsightsContext) => void; | ||
clearChat?: () => void; | ||
tellMoreAboutInsight?: (context: ProactiveInsightsContext) => void; | ||
}; | ||
export const AssistantActionsContext = | ||
createContext<AssistantActionsContextType>({ | ||
|
@@ -62,29 +65,21 @@ export const AssistantActionsContext = | |
clearChat: () => {}, | ||
}); | ||
|
||
export function useAssistantActions(): AssistantActionsContextType & { | ||
isAssistantEnabled: boolean; | ||
} { | ||
const isAssistantEnabled = usePreference('enableAIAssistant'); | ||
export function useAssistantActions(): AssistantActionsContextType { | ||
const isAIFeatureEnabled = useIsAIFeatureEnabled(); | ||
const isAssistantFlagEnabled = usePreference('enableAIAssistant'); | ||
const actions = useContext(AssistantActionsContext); | ||
if (!isAIFeatureEnabled || !isAssistantFlagEnabled) { | ||
return {}; | ||
} | ||
|
||
return { | ||
...useContext(AssistantActionsContext), | ||
isAssistantEnabled, | ||
}; | ||
return actions; | ||
} | ||
|
||
export const compassAssistantServiceLocator = createServiceLocator(function () { | ||
const { isAssistantEnabled, ...actions } = useAssistantActions(); | ||
const actions = useAssistantActions(); | ||
|
||
const assistantEnabledRef = useRef(isAssistantEnabled); | ||
assistantEnabledRef.current = isAssistantEnabled; | ||
|
||
return { | ||
...actions, | ||
getIsAssistantEnabled() { | ||
return assistantEnabledRef.current; | ||
}, | ||
}; | ||
return actions; | ||
}, 'compassAssistantLocator'); | ||
|
||
export type CompassAssistantService = ReturnType< | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I swear this gets either added or removed every other PR 😆 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am rebasing so much everything feels like a deja-vu 😆 |
||
logger.log.error( | ||
logger.mongoLogId(1_001_000_370), | ||
'Assistant', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,10 +41,8 @@ export function getConnectingStatusText(connectionInfo: ConnectionInfo) { | |
type ConnectionErrorToastBodyProps = { | ||
info?: ConnectionInfo | null; | ||
error: Error; | ||
showReviewButton: boolean; | ||
showDebugButton: boolean; | ||
onReview: () => void; | ||
onDebug: () => void; | ||
onReview?: () => void; | ||
onDebug?: () => void; | ||
}; | ||
|
||
const connectionErrorToastStyles = css({ | ||
|
@@ -94,8 +92,6 @@ const debugActionStyles = css({ | |
function ConnectionErrorToastBody({ | ||
info, | ||
error, | ||
showReviewButton, | ||
showDebugButton, | ||
onReview, | ||
onDebug, | ||
}: ConnectionErrorToastBodyProps): React.ReactElement { | ||
|
@@ -111,7 +107,7 @@ function ConnectionErrorToastBody({ | |
<span data-testid="connection-error-text">{error.message}</span> | ||
</span> | ||
<span className={connectionErrorActionsStyles}> | ||
{info && showReviewButton && ( | ||
{info && onReview && ( | ||
<span> | ||
<Button | ||
onClick={onReview} | ||
|
@@ -122,7 +118,7 @@ function ConnectionErrorToastBody({ | |
</Button> | ||
</span> | ||
)} | ||
{info && showDebugButton && ( | ||
{info && onDebug && ( | ||
<span className={debugActionStyles}> | ||
<Icon glyph="Sparkle" size="small"></Icon> | ||
<Link | ||
|
@@ -182,8 +178,6 @@ const openConnectionSucceededToast = (connectionInfo: ConnectionInfo) => { | |
const openConnectionFailedToast = ({ | ||
connectionInfo, | ||
error, | ||
showReviewButton, | ||
showDebugButton, | ||
onReviewClick, | ||
onDebugClick, | ||
}: { | ||
|
@@ -192,10 +186,8 @@ const openConnectionFailedToast = ({ | |
// can happen is autoconnect flow | ||
connectionInfo: ConnectionInfo | null | undefined; | ||
error: Error; | ||
showReviewButton: boolean; | ||
showDebugButton: boolean; | ||
onReviewClick: () => void; | ||
onDebugClick: () => void; | ||
onReviewClick?: () => void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
onDebugClick?: () => void; | ||
}) => { | ||
const failedToastId = connectionInfo?.id ?? 'failed'; | ||
|
||
|
@@ -206,19 +198,19 @@ const openConnectionFailedToast = ({ | |
<ConnectionErrorToastBody | ||
info={connectionInfo} | ||
error={error} | ||
showReviewButton={showReviewButton} | ||
showDebugButton={showDebugButton} | ||
onReview={() => { | ||
if (!showDebugButton) { | ||
// don't close the toast if there are two actions so that the user | ||
// can still use the other one | ||
closeToast(`connection-status--${failedToastId}`); | ||
} | ||
onReviewClick(); | ||
}} | ||
onDebug={() => { | ||
onDebugClick(); | ||
}} | ||
onReview={ | ||
onReviewClick | ||
? () => { | ||
if (!onDebugClick) { | ||
// don't close the toast if there are two actions so that the user | ||
// can still use the other one | ||
closeToast(`connection-status--${failedToastId}`); | ||
} | ||
onReviewClick(); | ||
} | ||
: undefined | ||
} | ||
onDebug={onDebugClick} | ||
/> | ||
), | ||
variant: 'warning', | ||
|
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.