Skip to content
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

Merged
merged 16 commits into from
Oct 23, 2024
Merged

Conversation

fbelginetw
Copy link
Collaborator

@fbelginetw fbelginetw commented Oct 21, 2024

Overview

This PR refactors Opentrons AI Client to match the new design. It also adds the mixpanel files for tracking analytics.

image

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

  • New Opentron AI client flow and Landing page.
  • Add mixpanel analytics.

Review requests

  • Landing page.
  • Verify if the approach for Mixpanel implementation is ok.

Risk assessment

It breaks the current Opentrons AI client, as it's being remade from the ground up.

@fbelginetw fbelginetw marked this pull request as ready for review October 23, 2024 18:27
@fbelginetw fbelginetw requested a review from a team as a code owner October 23, 2024 18:27
@fbelginetw fbelginetw changed the title [WIP] feat: Opentrons ai client landing page feat: Opentrons ai client landing page Oct 23, 2024
Copy link
Member

@shlokamin shlokamin left a 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)
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

Copy link
Member

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
Copy link
Member

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
Copy link
Member

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?

@fbelginetw fbelginetw merged commit 1deeb88 into edge Oct 23, 2024
8 checks passed
@fbelginetw fbelginetw deleted the opentrons-ai-client-landing-page branch October 23, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants