Repository navigation
fix(backend): treat GitHub PRs with merged_at as merged - #309
Merged
stefan-ernst merged 2 commits intoOct 5, 2026
Merged
Conversation
GitHub's list endpoint (GET /repos/{owner}/{repo}/pulls), which the SCM
sync uses, omits the `merged` field and only returns `merged_at`. Every
listed PR therefore converted with IsMerged = false, so sync stored merged
PRs as "closed", never emitted scm_pr_merged, and skipped smart commits.
Count a non-nil MergedAt as merged as well. The single-PR endpoint keeps
working as before because it sets both fields.
Fixes Windshiftapp#304
|
All contributors have signed the CLA ✍️ ✅ |
Contributor
Author
|
I have read the CLA Document and I hereby sign the CLA |
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
2 participants
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.
Fixes #304.
What
githubPullRequest.toPullRequest() now sets IsMerged from merged || merged_at != nil instead of merged alone.
Why
SCM sync lists PRs with GET /repos/{owner}/{repo}/pulls. GitHub's list response has no merged field, only merged_at, so every listed PR converted with IsMerged = false. Sync then stored merged PRs as closed, skipped emitPRMergedEvent, and never ran smart commits. The single-PR endpoint (GetPullRequest) does return merged, which is why a link later flips to "Merged" on refresh.
This matches the GitLab provider, which already uses state == "merged" || MergedAt != nil. Gitea's list endpoint returns merged, so only the GitHub provider changes.
Tests
internal/scm/github_pull_request_test.go (with a .gitignore exception, like gitlab_test.go) drives ListPullRequests through a fake transport with list-shaped JSON and checks merged, closed-unmerged and open PRs, plus the existing merged: true path.
I couldn't run the Go suite locally against Go 1.27, so please let CI and core-tests confirm.
Not in this PR
The refresh path in #304 is still open. updateLinkFromProvider writes state = merged without emitting scm_pr_merged. If a user refreshes a link between the merge and the next sync, sync then sees previousState == merged and doesn't emit the event either. This fix makes the race narrow, because sync now detects merges on its own. Closing it fully means routing the refresh through upsertPullRequestSCMLink, or emitting from the refresh path. I can follow up with whichever approach you prefer.