-
Notifications
You must be signed in to change notification settings - Fork 23
feat(integrations): wire github finish setup from posthog code #2215
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
base: main
Are you sure you want to change the base?
Changes from all commits
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,51 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { describe, expect, it } from "vitest"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| githubInstallationSettingsUrl, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolveGithubInstallationId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } from "./githubInstallationSettingsUrl"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| describe("githubInstallationSettingsUrl", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it("uses org settings for organization accounts", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| githubInstallationSettingsUrl("99", { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type: "Organization", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: "posthog", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ).toBe( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "https://github.com/organizations/posthog/settings/installations/99", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it("uses user settings for personal accounts", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| githubInstallationSettingsUrl("42", { type: "User", name: "octocat" }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ).toBe("https://github.com/settings/installations/42"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| describe("resolveGithubInstallationId", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it("prefers top-level installation_id then id then config", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolveGithubInstallationId({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: 99, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| kind: "github", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| installation_id: "a", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config: { installation_id: "c" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ).toBe("a"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolveGithubInstallationId({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: 1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| kind: "github", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| integration_id: 12345, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ).toBe("12345"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolveGithubInstallationId({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: 1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| kind: "github", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config: { installation_id: "c" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ).toBe("c"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+26
to
+51
Contributor
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.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/code/src/renderer/features/integrations/utils/githubInstallationSettingsUrl.test.ts
Line: 26-51
Comment:
Three distinct input/output cases are bundled in a single `it` with multiple `expect` calls. The project explicitly prefers parameterised tests — if the first assertion fails the remaining cases are never run, making it harder to pinpoint regressions. Using `it.each` gives each case an independent result and a descriptive name.
```suggestion
describe("resolveGithubInstallationId", () => {
it.each([
[
"prefers top-level installation_id over integration_id and config",
{ id: 99, kind: "github", installation_id: "a", config: { installation_id: "c" } },
"a",
],
[
"falls back to integration_id when installation_id is absent",
{ id: 1, kind: "github", integration_id: 12345 },
"12345",
],
[
"falls back to config.installation_id as last resort",
{ id: 1, kind: "github", config: { installation_id: "c" } },
"c",
],
])("%s", (_label, input, expected) => {
expect(resolveGithubInstallationId(input as Parameters<typeof resolveGithubInstallationId>[0])).toBe(expected);
});
});
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import type { Integration } from "../stores/integrationStore"; | ||
|
|
||
| interface GithubInstallationAccount { | ||
| type?: string | null; | ||
| name?: string | null; | ||
| } | ||
|
|
||
| export function githubInstallationSettingsUrl( | ||
| installationId: string, | ||
| account?: GithubInstallationAccount | null, | ||
| ): string { | ||
| const accountType = account?.type; | ||
| const accountName = account?.name; | ||
| if ( | ||
| typeof accountType === "string" && | ||
| accountType.toLowerCase() === "organization" && | ||
| typeof accountName === "string" && | ||
| accountName | ||
| ) { | ||
| return `https://github.com/organizations/${accountName}/settings/installations/${installationId}`; | ||
| } | ||
| return `https://github.com/settings/installations/${installationId}`; | ||
| } | ||
|
|
||
| /** Resolves a GitHub App installation id from team or user integration payloads. */ | ||
| export function resolveGithubInstallationId( | ||
| integration: Integration, | ||
| ): string | null { | ||
| const legacy = integration as { | ||
| installation_id?: string | null; | ||
| integration_id?: string | number | null; | ||
| }; | ||
| const candidates = [ | ||
| legacy.installation_id, | ||
| legacy.integration_id, | ||
| integration.config?.installation_id, | ||
| ]; | ||
| for (const value of candidates) { | ||
| if (value === null || value === undefined) continue; | ||
| const id = String(value).trim(); | ||
| if (id) return id; | ||
| } | ||
| return null; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,14 @@ | ||
| import { useOptionalAuthenticatedClient } from "@features/auth/hooks/authClient"; | ||
| import { useAuthStateValue } from "@features/auth/hooks/authQueries"; | ||
| import { | ||
| describeGithubConnectError, | ||
| useGithubConnect, | ||
| } from "@features/integrations/hooks/useGithubUserConnect"; | ||
| import { useIntegrationSelectors } from "@features/integrations/stores/integrationStore"; | ||
| import { | ||
| githubInstallationSettingsUrl, | ||
| resolveGithubInstallationId, | ||
| } from "@features/integrations/utils/githubInstallationSettingsUrl"; | ||
| import { useRepositoryIntegration } from "@hooks/useIntegrations"; | ||
| import { | ||
| ArrowSquareOutIcon, | ||
|
|
@@ -11,13 +17,16 @@ import { | |
| InfoIcon, | ||
| } from "@phosphor-icons/react"; | ||
| import { Box, Button, Flex, Spinner, Text, Tooltip } from "@radix-ui/themes"; | ||
| import { openUrlInBrowser } from "@utils/browser"; | ||
|
|
||
| export function GitHubIntegrationSection({ | ||
| hasGithubIntegration, | ||
| }: { | ||
| hasGithubIntegration: boolean; | ||
| }) { | ||
| const { repositories, isLoadingRepos } = useRepositoryIntegration(); | ||
| const { githubIntegrations } = useIntegrationSelectors(); | ||
| const client = useOptionalAuthenticatedClient(); | ||
| const projectId = useAuthStateValue((state) => state.projectId); | ||
| const { | ||
| error: connectError, | ||
|
|
@@ -30,6 +39,25 @@ export function GitHubIntegrationSection({ | |
| projectHasTeamIntegration: hasGithubIntegration, | ||
| }); | ||
|
|
||
| const handleUpdateInGitHub = async () => { | ||
| const integration = githubIntegrations[0]; | ||
| if (!integration || projectId === null || !client) return; | ||
| const installationId = resolveGithubInstallationId(integration); | ||
| if (!installationId) return; | ||
| const nextPath = `/account-connected/github-integration?provider=github&project_id=${projectId}&connect_from=posthog_code`; | ||
| try { | ||
| await client.prepareGithubTeamIntegrationCallback(projectId, nextPath); | ||
| } catch { | ||
| return; | ||
| } | ||
|
Comment on lines
+48
to
+52
Contributor
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.
When Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/code/src/renderer/features/settings/components/sections/GitHubIntegrationSection.tsx
Line: 48-52
Comment:
**Silent failure gives no user feedback**
When `prepareGithubTeamIntegrationCallback` throws, the `catch` block silently returns. The user clicks "Update in GitHub" and nothing happens — no GitHub tab opens, no error message appears. At minimum the error should be surfaced (e.g. a toast), otherwise users are left believing the click registered when it didn't.
How can I resolve this? If you propose a fix, please make it concise. |
||
| void openUrlInBrowser( | ||
| githubInstallationSettingsUrl( | ||
| installationId, | ||
| integration.config?.account, | ||
| ), | ||
| ); | ||
| }; | ||
|
|
||
| return ( | ||
| <Flex | ||
| align="center" | ||
|
|
@@ -97,7 +125,11 @@ export function GitHubIntegrationSection({ | |
| weight="fill" | ||
| className="text-(--green-9)" | ||
| /> | ||
| <Button size="1" variant="soft" onClick={() => void handleConnect()}> | ||
| <Button | ||
| size="1" | ||
| variant="soft" | ||
| onClick={() => void handleUpdateInGitHub()} | ||
| > | ||
| Update in GitHub | ||
| <ArrowSquareOutIcon size={12} /> | ||
| </Button> | ||
|
|
||
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.
response.okis alreadytruefor every 2xx response including 204, so&& response.status !== 204can never change the outcome of the outer!response.okcheck. Per simplicity rule 4, this is a superfluous part.Prompt To Fix With AI