-
Notifications
You must be signed in to change notification settings - Fork 126
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: Add and edit github connection #5263
Changes from 10 commits
7c7c0f5
f157c1e
3caebf5
708237b
7028a51
c0a5baf
8f8879e
64e2d30
c57aa21
ec24980
546266e
53c1b1a
fc2bce3
5c3358f
08b0376
21afe75
9012e9f
1139185
58ca0c3
f402534
9a99b5c
b5d6cdc
61504ea
af24d8b
69e8508
88d387e
83d60f8
079b2b1
ce868ee
f09758d
be44162
6f8caba
0c17b50
f31a127
880f729
ce5ea9b
bfeca3b
f6c95a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
<script lang="ts"> | ||
import { | ||
AlertDialog, | ||
AlertDialogContent, | ||
AlertDialogDescription, | ||
AlertDialogFooter, | ||
AlertDialogHeader, | ||
AlertDialogTitle, | ||
AlertDialogTrigger, | ||
} from "@rilldata/web-common/components/alert-dialog"; | ||
import { Button } from "@rilldata/web-common/components/button"; | ||
import Github from "@rilldata/web-common/components/icons/Github.svelte"; | ||
|
||
export let open = false; | ||
export let onContinue: () => void; | ||
</script> | ||
|
||
<AlertDialog bind:open> | ||
<AlertDialogTrigger asChild> | ||
<div class="hidden"></div> | ||
</AlertDialogTrigger> | ||
<AlertDialogContent> | ||
<div class="flex flex-row gap-x-2"> | ||
<Github size="28px" /> | ||
<div class="flex flex-col"> | ||
<AlertDialogHeader> | ||
<AlertDialogTitle> | ||
Connect this project to a Github repo | ||
</AlertDialogTitle> | ||
<AlertDialogDescription> | ||
Before continuing, you need to make sure your Rill project has been | ||
<!-- TODO: docs link --> | ||
pushed to a repo on Github. | ||
<a href="https://docs.rilldata.com">Learn how</a> | ||
</AlertDialogDescription> | ||
</AlertDialogHeader> | ||
<AlertDialogFooter class="mt-5"> | ||
<Button type="secondary" on:click={() => (open = false)}> | ||
Cancel | ||
</Button> | ||
<Button type="primary" on:click={onContinue}>Continue</Button> | ||
</AlertDialogFooter> | ||
</div> | ||
</div> | ||
</AlertDialogContent> | ||
</AlertDialog> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import { | ||
createAdminServiceGetGithubUserStatus, | ||
getAdminServiceGetGithubUserStatusQueryKey, | ||
} from "@rilldata/web-admin/client"; | ||
import { queryClient } from "@rilldata/web-common/lib/svelte-query/globalQueryClient"; | ||
import { waitUntil } from "@rilldata/web-common/lib/waitUtils"; | ||
import { get } from "svelte/store"; | ||
|
||
export class GithubConnection { | ||
public readonly userStatus = createAdminServiceGetGithubUserStatus(); | ||
|
||
private connecting: boolean; | ||
|
||
public constructor(private readonly onReconnect: () => void) {} | ||
|
||
public async check() { | ||
this.connecting = false; | ||
await waitUntil(() => !get(this.userStatus).isLoading); | ||
const userStatus = get(this.userStatus).data; | ||
if (userStatus.hasAccess) { | ||
return this.onReconnect(); | ||
} | ||
|
||
this.connecting = true; | ||
window.open(userStatus.grantAccessUrl, "_blank"); | ||
} | ||
|
||
public async focused() { | ||
if (!this.connecting) return; | ||
await queryClient.refetchQueries( | ||
getAdminServiceGetGithubUserStatusQueryKey(), | ||
); | ||
if (this.connecting) this.onReconnect(); | ||
this.connecting = false; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,135 @@ | ||
<script lang="ts"> | ||
import { | ||
createAdminServiceGetProject, | ||
createAdminServiceUpdateProject, | ||
getAdminServiceGetGithubUserStatusQueryKey, | ||
} from "@rilldata/web-admin/client"; | ||
import { GithubRepoUpdater } from "@rilldata/web-admin/features/projects/github/GithubRepoUpdater"; | ||
import { | ||
AlertDialog, | ||
AlertDialogContent, | ||
AlertDialogDescription, | ||
AlertDialogFooter, | ||
AlertDialogHeader, | ||
AlertDialogTitle, | ||
AlertDialogTrigger, | ||
} from "@rilldata/web-common/components/alert-dialog"; | ||
import { Button } from "@rilldata/web-common/components/button"; | ||
import Github from "@rilldata/web-common/components/icons/Github.svelte"; | ||
import Select from "@rilldata/web-common/components/forms/Select.svelte"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A little out-of-scope of this PR, but it'd be nice to deprecate this in favor of ShadCN's Select component |
||
import Spinner from "@rilldata/web-common/features/entity-management/Spinner.svelte"; | ||
import { EntityStatus } from "@rilldata/web-common/features/entity-management/types"; | ||
import { eventBus } from "@rilldata/web-common/lib/event-bus/event-bus"; | ||
import { queryClient } from "@rilldata/web-common/lib/svelte-query/globalQueryClient"; | ||
import { invalidateRuntimeQueries } from "@rilldata/web-common/runtime-client/invalidation"; | ||
import type { AxiosError } from "axios"; | ||
|
||
export let open = false; | ||
export let project: string; | ||
export let organization: string; | ||
|
||
let githubUrl = ""; | ||
const githubRepoUpdater = new GithubRepoUpdater(); | ||
const githubRepos = githubRepoUpdater.userRepos; | ||
const status = githubRepoUpdater.status; | ||
const projectQuery = createAdminServiceGetProject(organization, project); | ||
|
||
$: repoSelections = | ||
$githubRepos.data?.repos?.map((r) => ({ | ||
value: r.url, | ||
label: `${r.owner}/${r.name}`, | ||
})) ?? []; | ||
|
||
const updateProject = createAdminServiceUpdateProject(); | ||
async function updateGithubUrl() { | ||
await $updateProject.mutateAsync({ | ||
name: project, | ||
organizationName: organization, | ||
data: { | ||
githubUrl, | ||
}, | ||
}); | ||
eventBus.emit("notification", { | ||
message: `Set github repo to ${githubUrl}`, | ||
type: "success", | ||
}); | ||
void queryClient.refetchQueries( | ||
getAdminServiceGetGithubUserStatusQueryKey(), | ||
); | ||
void invalidateRuntimeQueries( | ||
queryClient, | ||
$projectQuery.data.prodDeployment.runtimeInstanceId, | ||
); | ||
open = false; | ||
} | ||
|
||
function handleVisibilityChange() { | ||
if (document.visibilityState !== "visible") return; | ||
void githubRepoUpdater.focused(); | ||
} | ||
|
||
$: error = ($status.error ?? $updateProject.error) as unknown as AxiosError; | ||
</script> | ||
|
||
<svelte:window on:visibilitychange={handleVisibilityChange} /> | ||
|
||
<AlertDialog bind:open> | ||
<AlertDialogTrigger asChild> | ||
<div class="hidden"></div> | ||
</AlertDialogTrigger> | ||
<AlertDialogContent> | ||
<div class="flex flex-row gap-x-2"> | ||
<Github size="28px" /> | ||
<div class="flex flex-col"> | ||
<AlertDialogHeader> | ||
<AlertDialogTitle>Select Github repository</AlertDialogTitle> | ||
<AlertDialogDescription class="flex flex-col gap-y-1"> | ||
<span> | ||
Which Github repo would you like to connect to this Rill project? | ||
</span> | ||
{#if $status.isLoading} | ||
<div class="flex flex-row items-center ml-5 h-8"> | ||
<Spinner status={EntityStatus.Running} /> | ||
</div> | ||
{:else} | ||
<Select | ||
id="repo-selector" | ||
bind:value={githubUrl} | ||
label="" | ||
options={repoSelections} | ||
/> | ||
{/if} | ||
<span class="font-semibold"> | ||
Note: Contents of this repo will replace your current Rill | ||
project. | ||
</span> | ||
{#if error} | ||
<div class="text-red-500 text-sm py-px"> | ||
{error.response?.data?.message ?? error.message} | ||
</div> | ||
{/if} | ||
</AlertDialogDescription> | ||
</AlertDialogHeader> | ||
<AlertDialogFooter class="mt-5"> | ||
<Button | ||
outline={false} | ||
type="link" | ||
on:click={() => githubRepoUpdater.check()} | ||
> | ||
Choose other repos | ||
</Button> | ||
<Button type="secondary" on:click={() => (open = false)}> | ||
Cancel | ||
</Button> | ||
<Button | ||
type="primary" | ||
on:click={() => updateGithubUrl()} | ||
loading={$updateProject.isLoading} | ||
> | ||
Continue | ||
</Button> | ||
</AlertDialogFooter> | ||
</div> | ||
</div> | ||
</AlertDialogContent> | ||
</AlertDialog> |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, is there a reason to be adding this abstraction on top of vanilla TanStack Query? To update the GitHub repo seems like a vanilla mutation and shouldn't require too much complexity. I wonder if the bugginess I shared via Jam is a derivative of this complexity. If we're to go down this route, I'd expect a "GithubRepoUpdater" to include a method to actually update the Github repo, but that looks to be happening outside of this abstraction in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This endpoint can be slow if there are a lot of repos and orgs connected. I want to avoid refetching it on window focus all the time, but only when Both this and GithubConnection have a check to only refetch if it was explicitly asked for (basically the Naming this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. The vanilla TanStack approach would be to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are refetching the query right? The idea is to hide as much code outside of components as possible. We still need the check if we popped a page to select github repos or not. This will also hopefully allow us to abstract such code out. But it will have to be follow ups |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
import { | ||
adminServiceListGithubUserRepos, | ||
createAdminServiceGetGithubUserStatus, | ||
createAdminServiceListGithubUserRepos, | ||
getAdminServiceListGithubUserReposQueryKey, | ||
type RpcStatus, | ||
} from "@rilldata/web-admin/client"; | ||
import { queryClient } from "@rilldata/web-common/lib/svelte-query/globalQueryClient"; | ||
import { waitUntil } from "@rilldata/web-common/lib/waitUtils"; | ||
import { derived, get } from "svelte/store"; | ||
|
||
export class GithubRepoUpdater { | ||
public readonly userStatus = createAdminServiceGetGithubUserStatus(); | ||
public readonly userRepos = derived([this.userStatus], ([userStatus], set) => | ||
createAdminServiceListGithubUserRepos({ | ||
query: { | ||
enabled: !!userStatus.data?.hasAccess, | ||
queryClient, | ||
}, | ||
}).subscribe(set), | ||
) as ReturnType< | ||
typeof createAdminServiceListGithubUserRepos< | ||
Awaited<ReturnType<typeof adminServiceListGithubUserRepos>>, | ||
RpcStatus | ||
> | ||
>; | ||
|
||
public readonly status = derived( | ||
[this.userStatus, this.userRepos], | ||
([userStatus, userRepos]) => { | ||
if (userStatus.isFetching || userRepos.isFetching) { | ||
return { | ||
isLoading: true, | ||
error: undefined, | ||
}; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be consistent with terminology. If reading |
||
|
||
return { | ||
isLoading: false, | ||
error: userStatus.error ?? userRepos.error, | ||
}; | ||
}, | ||
); | ||
|
||
private connecting: boolean; | ||
|
||
public constructor() {} | ||
|
||
public async check() { | ||
this.connecting = false; | ||
await waitUntil(() => !get(this.userStatus).isLoading); | ||
const userStatus = get(this.userStatus).data; | ||
|
||
this.connecting = true; | ||
window.open(userStatus.grantAccessUrl, "_blank"); | ||
} | ||
|
||
public async focused() { | ||
if (!this.connecting) return; | ||
await queryClient.refetchQueries( | ||
getAdminServiceListGithubUserReposQueryKey(), | ||
); | ||
this.connecting = 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.
IMO this extra layer of abstraction isn't necessary & adds complexity.
Is there something wrong with using vanilla TanStack Query in the
ProjectGitHubConnection.svelte
component? That's the pattern we're using most frequently throughout the codebase.