Only create merge history entries on the actual merge event #15

Merged

arne merged 1 commit from fix/merge-history-on-edit into main

2026-04-05 22:18:16 +02:00

arne commented

2026-04-05 22:15:40 +02:00

Owner

The PR dispatch in handleWebhook used to fire handlePRMerge whenever pr.Merged was true, without checking the webhook action. A merged PR stays merged=true forever, so any subsequent webhook event on it — a title change, a body edit, a label tweak, a comment — would re-fire the handler and insert another history row for the same merge.

We noticed this while rewriting historical PR titles via the Forgejo API: each PATCH to a merged PR triggered an edited webhook, which produced a duplicate history entry. Seven PRs ended up with fourteen spurious rows in orbit's live DB before the pattern was clear. The duplicates were cleaned up manually via SQL, but the underlying bug remained.

The fix

One-line guard on the dispatch. Require action == "closed" alongside pr.Merged, so the handler runs only on the actual merge event:

if payload.Action == "closed" && payload.PullRequest.Merged {
    s.handlePRMerge(project, payload.PullRequest)
}

The PR row itself still updates via handlePREvent on every webhook delivery — that runs unconditionally and is idempotent by design, so PR metadata in the store stays in sync with Forgejo regardless of which action fired.

Regression test

TestWebhookPR_EditedAfterMerge in api/webhooks_test.go exercises the bug directly: merge a PR, assert one history entry exists, send a follow-up edited webhook on the same (still-merged) PR, assert the history entry count is still one. The test also verifies the PR row picked up the edit via handlePREvent — so we're confirming the fix doesn't over-correct and suppress legitimate state updates.

Follow-ups

None. The fix is surgical and the test covers the exact scenario that produced the duplicates.

The PR dispatch in `handleWebhook` used to fire `handlePRMerge` whenever `pr.Merged` was true, without checking the webhook action. A merged PR stays `merged=true` forever, so any subsequent webhook event on it — a title change, a body edit, a label tweak, a comment — would re-fire the handler and insert another history row for the same merge. We noticed this while rewriting historical PR titles via the Forgejo API: each PATCH to a merged PR triggered an `edited` webhook, which produced a duplicate history entry. Seven PRs ended up with fourteen spurious rows in orbit's live DB before the pattern was clear. The duplicates were cleaned up manually via SQL, but the underlying bug remained. ## The fix One-line guard on the dispatch. Require `action == "closed"` alongside `pr.Merged`, so the handler runs only on the actual merge event: ```go if payload.Action == "closed" && payload.PullRequest.Merged { s.handlePRMerge(project, payload.PullRequest) } ``` The PR row itself still updates via `handlePREvent` on every webhook delivery — that runs unconditionally and is idempotent by design, so PR metadata in the store stays in sync with Forgejo regardless of which action fired. ## Regression test `TestWebhookPR_EditedAfterMerge` in `api/webhooks_test.go` exercises the bug directly: merge a PR, assert one history entry exists, send a follow-up `edited` webhook on the same (still-merged) PR, assert the history entry count is still one. The test also verifies the PR row picked up the edit via `handlePREvent` — so we're confirming the fix doesn't over-correct and suppress legitimate state updates. ## Follow-ups None. The fix is surgical and the test covers the exact scenario that produced the duplicates.

arne added 1 commit

2026-04-05 22:15:40 +02:00

Only create merge history entries on the actual merge event b107f76e67

The PR dispatch in handleWebhook used to fire handlePRMerge whenever
pr.Merged was true, without checking the webhook action. A merged PR
stays merged=true forever, so any subsequent webhook event on it — a
title change, a body edit, a label tweak, a comment — would re-fire the
handler and insert another history row for the same merge.

We noticed this while rewriting historical PR titles via the Forgejo
API: each PATCH to a merged PR triggered an 'edited' webhook, which
produced a duplicate history entry. Seven PRs ended up with fourteen
spurious rows before the pattern was clear.

The fix is a one-line guard on the dispatch: require action == 'closed'
alongside pr.Merged, so the handler runs only on the actual merge event.

Adds a regression test that merges a PR, confirms the single expected
history entry lands, then sends a follow-up 'edited' webhook on the
same (still-merged) PR and verifies no new history entry was created.
The PR row itself still updates via handlePREvent, which runs
unconditionally and is idempotent by design.