-
Notifications
You must be signed in to change notification settings - Fork 179
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
feat: Opentrons ai client landing page #16552
Conversation
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.
lgtm, just some minor comments so feel free to address later. is the plan to implement functionality to toggle analytics opt in later?
const { getAccessToken } = useGetAccessToken() | ||
const trackEvent = useTrackEvent() | ||
|
||
initializeMixpanel(mixpanel) |
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 should probably be done outside of the component so we don't initialize mixpanel multiple times right? otherwise we'll call this on every render. maybe we can have a single initialize function that gets called at the root of the project or something
import type { RouteProps } from './resources/types' | ||
|
||
const opentronsAIRoutes: RouteProps[] = [ | ||
// replace Landing with the correct component |
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 this meant to be done later?
import { getHasOptedIn } from './selectors' | ||
|
||
export const getIsProduction = (): boolean => | ||
global.location.host === 'designer.opentrons.com' // UPDATE THIS TO CORRECT URL |
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 this should be ai.opentrons.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.
also is this being used anywhere? i dont think we need this since the correct mixpanel token gets injected at build time
// Register "super properties" which are included with all events | ||
mixpanel.register({ | ||
appVersion: 'test', // TODO update this? | ||
// NOTE(IL, 2020): Since PD may be in the same Mixpanel project as other OT web apps, this 'appName' property is intended to distinguish 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.
lets remove this comment, its only relevant to PD
|
||
/** ChatDataAtom is for chat data (user prompt and response from OpenAI API) */ | ||
export const chatDataAtom = atom<ChatData[]>([]) | ||
|
||
export const chatHistoryAtom = atom<Chat[]>([]) | ||
|
||
export const tokenAtom = atom<string | null>(null) | ||
|
||
export const mixpanelAtom = atom<Mixpanel | null>({ | ||
analytics: { hasOptedIn: true }, // TODO: set to false |
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 should be false right?
Overview
This PR refactors Opentrons AI Client to match the new design. It also adds the mixpanel files for tracking analytics.
Test Plan and Hands on Testing
Still wip, but tested manually the landing page and its buttons, and if mixpanel is tracking the events.
Changelog
Review requests
Risk assessment
It breaks the current Opentrons AI client, as it's being remade from the ground up.