Fix incorrect first-interaction detection#410
Open
t-hamano wants to merge 4 commits into
Open
Conversation
Evaluate only the check matching the event type when deciding whether this is the sender's first contribution: an issue event looks at issue history, a PR event at pull request history. PR actions#311 (TypeScript conversion) replaced the original event-branching logic with a combined `isFirstIssue() && isFirstPullRequest()` condition, which posts the greeting when *either* is the sender's first. When issues are disabled, isFirstIssue() always returns true, so every PR (even from repeat contributors) was greeted. The symmetric case (a sender with no PRs always greeted on issues) was also broken. Add regression tests covering both directions. Co-Authored-By: Claude <noreply@anthropic.com>
t-hamano
changed the title
t-hamano
changed the title
There was a problem hiding this comment.
Pull request overview
This PR restores the original “first interaction” semantics by checking only the contribution type that matches the triggering event (issue vs pull request), preventing repeat contributors from being incorrectly greeted when only one feature (issues/PRs) is enabled.
Changes:
- Update first-contribution detection to evaluate only
isFirstIssuefor issue events and onlyisFirstPullRequestfor PR events. - Regenerate
dist/index.jsto reflect the source change. - Expand/adjust unit tests to cover event-specific first-contribution scenarios.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/main.ts |
Restores event-specific first-interaction detection logic (issue vs PR). |
dist/index.js |
Updates the built artifact to match src/ behavior changes. |
__tests__/main.test.ts |
Adds/updates tests for event-specific skipping behavior (but needs tightening to catch regressions). |
Comments suppressed due to low confidence (1)
tests/main.test.ts:96
- This test now describes “prior issues and PRs”, but
run()only evaluates the first-contribution check that matches the event type (issue vs PR). Because the default test context is a PR event, the current setup (mocking twopaginatecalls) can still pass even if a regression reintroduces calling both checks. Rename the test to match the PR-only behavior and assert the correct API call/count.
it('Skips a message if the sender has prior issues and PRs', async () => {
mocktokit.paginate
// Issues
.mockResolvedValueOnce([
{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Address review feedback: the PR-event regression test mocked an unused issues paginate call and only checked the log message. Remove the unused mock and assert that paginate is called exactly once with pulls.list, so the test fails if both checks are accidentally reintroduced for PR events. Co-Authored-By: Claude <noreply@anthropic.com>
|
Fixes #369. @ncalteen 👋 Tagging as the person who has maintained this repository most recently based on commit activity. Would it be possible to get a review for this, or assigned to someone who can? We're using it in the https://github.com/WordPress/wordpress-develop/ repository (which does not use issues) and it is causing a good amount of noise. Thanks in advance! |
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
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.
Problem
This PR fixes an issue where the action incorrectly detects a "first interaction" and greets contributors who are not actually first-time contributors.
This conditional statement was modified by #311, which appears to be unintentional.
Before:
https://github.com/actions/first-interaction/pull/311/changes#diff-7934bf411fea192ad8cd69e0a12911648a2842cb0f2409a8fb67b41b7069d757L62-L67
After:
https://github.com/actions/first-interaction/pull/311/changes#diff-4fab5baaca5c14d2de62d8d2fceef376ddddcc8e9509d86cfa5643f51b89ce3dR46
This logic change will cause
isFirstIssueto always be true if the issue feature is disabled on the repository, consequently adding a message to contributors who have already submitted a PR.Approach
Restore the original behavior: evaluate only the check matching the event type. As a result, both the first issue and the first pull request are considered first contributions. This is the same specification as before the action was converted from a container action.