Add detection for stacked PRs across forks error in GitHub API responses #10654
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Copilot
AI
changed the title
…edentials Co-authored-by: Byron <[email protected]>
Byron
force-pushed
the
copilot/check-failure-for-stacked-prs
branch
from
8386d65 to
9371137
Compare
|
@estib-vega I took a good look at the code and it seems sound, while solving the stated problem at least according to the tests. I don't know though if this is the right approach at all. Is this salvageable? |
| test('should not detect stacked PR error for other validation errors', async () => { | ||
| const mockError = { | ||
| message: 'Validation Failed', | ||
| response: { | ||
| data: { | ||
| message: 'Validation Failed', | ||
| errors: [ | ||
| { | ||
| resource: 'PullRequest', | ||
| field: 'title', | ||
| code: 'missing' | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| vi.spyOn(octokit.pulls, 'create').mockRejectedValue(mockError); | ||
|
|
||
| try { | ||
| await service?.createPr({ | ||
| title: 'Test PR', | ||
| body: 'Test body', | ||
| draft: false, | ||
| baseBranchName: 'feature-branch', | ||
| upstreamName: 'my-branch' | ||
| }); | ||
| expect.fail('Should have thrown an error'); | ||
| } catch (err: any) { | ||
| expect(err.code).not.toBe(Code.GitHubStackedPrFork); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Perhaps we could manually remove this test case, it feels unnecessary.
| const message = isErrorlike(err) ? err.message : String(err); | ||
| const code = message.startsWith('Not Found -') ? Code.GitHubTokenExpired : undefined; | ||
|
|
||
| // Check for stacked PR across forks error (base field invalid) | ||
| let code: string | undefined; | ||
| if (isErrorlike(err) && 'response' in err) { | ||
| const response = (err as any).response; | ||
| if (response?.data?.errors instanceof Array) { | ||
| const hasInvalidBaseError = response.data.errors.some( | ||
| (error: any) => | ||
| error.resource === 'PullRequest' && error.field === 'base' && error.code === 'invalid' | ||
| ); | ||
| if (hasInvalidBaseError) { | ||
| code = Code.GitHubStackedPrFork; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check for expired token | ||
| if (!code && message.startsWith('Not Found -')) { | ||
| code = Code.GitHubTokenExpired; | ||
| } |
There was a problem hiding this comment.
This block could be improved extracting a type guard called isGitHubError.
There was a problem hiding this comment.
@copilot Extract the type guard as isGitHubError.
There was a problem hiding this comment.
Extracted the type guard as isGitHubError with a proper GitHubErrorResponse interface. This improves type safety and makes the error checking logic more readable. Commit: Extract isGitHubError type guard as suggested
|
Thanks for the review @mtsgrd. I did what I could and hope the copilot comes back with the type guard extraction. Can I ask you to take it from there to get it merged if good? PS: The first time I see how this works, but looks like Copilot just runs as part of an action :): https://github.com/gitbutlerapp/gitbutler/actions/runs/18523828069/job/52789809376 (this particular one is also failing with 500 server error, maybe due to a vibe-coded server ;)). |
Co-authored-by: Byron <[email protected]>
3 participants
isGitHubErrortype guard to improve code clarityOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.