fix(integrations): Fix "Update in GitHub" flow#2215
Open
Twixes wants to merge 3 commits into
Open
Conversation
…s urls Add githubInstallationSettingsUrl helper and connect PostHog client finish_setup for GitHub App installation flow from notification settings.
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/code/src/renderer/features/settings/components/sections/GitHubIntegrationSection.tsx:48-52
**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.
### Issue 2 of 3
apps/code/src/renderer/features/integrations/utils/githubInstallationSettingsUrl.test.ts:26-51
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);
});
});
```
### Issue 3 of 3
apps/code/src/renderer/api/posthogClient.ts:664
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) {
```
Reviews (1): Last reviewed commit: "feat(integrations): wire github finish s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+48
to
+52
| try { | ||
| await client.prepareGithubTeamIntegrationCallback(projectId, nextPath); | ||
| } catch { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
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.
Comment on lines
+26
to
+51
| 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"); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
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!
| body: JSON.stringify({ next }), | ||
| }, | ||
| }); | ||
| if (!response.ok && response.status !== 204) { |
Contributor
There was a problem hiding this comment.
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.Keep repo tooltip summarization from main alongside finish_setup wiring.
Twixes
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's broken
If you already have GitHub connected and hit "Update in GitHub" in Code settings, it wasn't doing the right thing. It kicked off a fresh connect flow instead of taking you to your existing app installation. And when you came back after changing repo access on GitHub, Code didn't reliably pick up the update (esp. the
setup_action=updatepath where GitHub doesn't sendstate).What this fixes
"Update in GitHub" now opens the correct GitHub installation settings page (org installs vs personal installs are different URLs), and sets up the callback so PostHog knows to finish setup when you return. You land back in Code with refreshed integration state. Initial "Connect GitHub" is unchanged..
Needs PostHog/posthog#58951 on the backend for the return trip to actually work end-to-end.