-
Notifications
You must be signed in to change notification settings - Fork 291
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
Add prompts migration API #5954
Conversation
3d6e28a
to
747d5d3
Compare
@@ -747,6 +773,15 @@ export class SourcegraphGraphQLAPIClient { | |||
) | |||
} | |||
|
|||
public async getCurrentUserRole(): Promise<CurrentUserRoleResponse['currentUser'] | null | Error> { |
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.
Because the query only returns sideAdmin
this should rather be renamed to isCurrentUserSideAdmin
and should return Promise<boolean>
.
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 also have to have user's ID here to properly create prompt (owner ID field), so it's not pure admin check query
|
||
// Change prompt visibility to PUBLIC if it's admin performing migration | ||
// TODO: [VK] Remove it and use visibility field in prompt creation (current API limitation) | ||
if (currentUser.siteAdmin) { |
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.
In Jason's PR he is removing the isSiteAdmin
check in his PR. I am not sure what we want to do with this check yet.
draft: false, | ||
autoSubmit: false, | ||
mode: commandModeToPromptMode(command.mode), | ||
visibility: 'SECRET', |
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.
Can we not just set the visibility here only rather than transferring the ownership. How does the createPrompt mutation work? Is it allowed to do that? If not, then why is the visibility field even required if the default is SECRET
?
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 is tricky, and it looks like a BE API issue, so prompt mutation requires a visibility field, but regardless of the value we're passing, it always will create a secret private prompt. So this is why we have different mutation for making it public (which doesn't make much sense to me why we can just create a public prompt with crete mutation)
This tricky with transferPromptOwnership
exists here only as a workaround for this API limitation.
@@ -15,7 +16,8 @@ export const PromptsTab: React.FC<{ | |||
const runAction = useActionSelect() | |||
|
|||
return ( | |||
<div className="tw-overflow-auto tw-h-full"> | |||
<div className="tw-overflow-auto tw-h-full tw-flex tw-flex-col tw-gap-6"> |
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 the PromptsTab rendered for only the upcoming version of SG? If not, should we check for the SG version here so that users connected to old sg instance are not shown this?
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.
That is very good point; I put this behind the feature flag code-unified-prompts
, and also, getPromptsMigrationInfo
handles this case when we're connected to lower versions than 5.9.0 (in this case we just skip means not rendering migration widget UI)
> | ||
<Progress.Indicator | ||
className={styles.loaderIndicator} | ||
style={{ |
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.
Lol, does the Progress component not handle the translate out of box? And if they don't, can we just extract it into a separate component in ui
folder and use it from there.
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 realized that during migration we also should transfer custom command context *by adding now supported mention syntax in the prompt message, just putting it here in case someone wants to merge this |
8dc5f20
to
d38593b
Compare
Part of SRCH-1162
Closes CODY-3052
This PR adds commands to prompt migration. We store commands locally, and this migration transfers commands to the prompts library (since the prompts library has got near feature parity with commands when it comes to chat submission and inline change execution)
Test plan