-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[OPIK-902] put ApiKey and workspace name to add experiment dialog #1167
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.
Overall code looks good, we can merge and improve later if needed :)
const ConfigureEnvCode: React.FC = () => { | ||
const { data: user } = useUser(); | ||
|
||
if (!user) return; |
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 know this case won't happen since this component won't get rendered until we have the user
but anyway, probably we should fallback here returning <ConfigureEnvCodeCore />
or a loading state so always this component returns a valid component?
If not the screen would look off the day this condition is an actual possibility
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.
As I see, we have WorkspacePreloader which loads and show loader until user data is loaded, this case is not possible and "if (!user) return;" works only as TS type guard, it will always return data
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 understand you added it due TS type guard but again, this component returns a code block or undefined
, the undefined
path is wrong, we're going to display a title + nothing + next block title
This case will never happen due what I mentioned above but that doesn't mean we should not build the components in the right way (like returning a loader instead of undefined
)
const getConfigCode = ( | ||
workspaceName: string, | ||
apiKey?: string, | ||
shouldMaskApiKey = false, | ||
) => { | ||
if (!apiKey) | ||
return `os.environ["OPIK_URL_OVERRIDE"] = "${window.location.origin}${BASE_API_URL}"`; | ||
|
||
const apiKeyConfig = `os.environ["OPIK_API_KEY"] = "${ | ||
shouldMaskApiKey ? maskAPIKey(apiKey) : apiKey | ||
}"`; | ||
const workspaceConfig = `os.environ["OPIK_WORKSPACE"] = "${workspaceName}"`; | ||
|
||
return `${apiKeyConfig} \n${workspaceConfig}`; | ||
}; |
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 we should aim to share this logic here and in the Quickstart page/side-popup
Details
Issues
Resolves #
Testing
Documentation