Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions apps/code/src/renderer/api/posthogClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,33 @@ export class PostHogAPIClient {
};
}

/** Seed team GitHub setup callback state before opening github.com installation settings. */
async prepareGithubTeamIntegrationCallback(
teamId: number,
next: string,
): Promise<void> {
const urlPath = `/api/environments/${teamId}/integrations/github/prepare_callback/`;
const url = new URL(`${this.api.baseUrl}${urlPath}`);
const response = await this.api.fetcher.fetch({
method: "post",
url,
path: urlPath,
overrides: {
body: JSON.stringify({ next }),
},
});
if (!response.ok && response.status !== 204) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Redundant status guard — response.ok is already true for every 2xx response including 204, so && response.status !== 204 can never change the outcome of the outer !response.ok check. Per simplicity rule 4, this is a superfluous part.

Suggested change
if (!response.ok && response.status !== 204) {
if (!response.ok) {
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/api/posthogClient.ts
Line: 664

Comment:
Redundant status guard — `response.ok` is already `true` for every 2xx response including 204, so `&& response.status !== 204` can never change the outcome of the outer `!response.ok` check. Per simplicity rule 4, this is a superfluous part.

```suggestion
    if (!response.ok) {
```

How can I resolve this? If you propose a fix, please make it concise.

const err = (await response.json().catch(() => ({}))) as {
detail?: unknown;
};
const detail =
typeof err.detail === "string"
? err.detail
: "Failed to prepare GitHub callback";
throw new Error(detail);
}
}

async getGithubUserIntegrations(): Promise<UserGitHubIntegration[]> {
const urlPath = `/api/users/@me/integrations/`;
const url = new URL(`${this.api.baseUrl}${urlPath}`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export interface IntegrationAccount {

export interface IntegrationConfig {
account?: IntegrationAccount;
installation_id?: string | number | null;
[key: string]: unknown;
}

Expand Down
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Suggested change
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");
});
});
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);
});
});
Prompt To Fix With AI
This 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,
Expand All @@ -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,
Expand All @@ -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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

Prompt To Fix With AI
This 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"
Expand Down Expand Up @@ -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>
Expand Down
Loading