-
Notifications
You must be signed in to change notification settings - Fork 188
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(generative-ai): move ai sign in modal to gen ai package from atlas-service COMPASS-8459 #6448
chore(generative-ai): move ai sign in modal to gen ai package from atlas-service COMPASS-8459 #6448
Conversation
signInWithModalPrompt, | ||
signInWithoutPrompt, | ||
} from './store/atlas-signin-reducer'; | ||
import { signInWithoutPrompt } from './store/atlas-signin-reducer'; |
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.
Similar to above, this naming should probably be adjusted, no point in calling out that there is no prompt if there is no other option in this service
@@ -4,7 +4,7 @@ import type { AtlasUserInfo } from './util'; | |||
export type ArgsWithSignal<T = Record<string, unknown>> = T & { | |||
signal?: AbortSignal; | |||
}; | |||
export type SignInPrompt = 'none' | 'ai-promo-modal'; | |||
export type SignInPrompt = 'none'; |
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.
We probably should consider how we can remote this type altogether now, I know that it's right now here so that the reducer can trigger the ipc without recursively calling itself, but this is probably more of a a flaw than a feature now that we don't have the second option
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.
Good callout, it could use an intentional function for each I feel. It's a bit overloaded without benefit at the moment. I'll see what we can do here.
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.
Related to other comment: not sure if keeping atlas-signin in the name / path here and in other moved files makes sense to be honest
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.
looks good to me!
…le-to-gen-ai-package
@gribnoysup Going to do your feedback in a follow up pr. I'd like to unblock https://jira.mongodb.org/browse/COMPASS-8378 which will use a lot of this code, and the plugin we're adding to compass-generative-ai. |
COMPASS-8459
No user facing changes.