Skip to content

Commit 86df148

Browse files
committed
fix: fetch live assignees before removal; isolate concurrency per reviewer
- In removeReviewerFromAssignees, replace the stale event-payload assignee snapshot with a live pulls.get call so a concurrent review_requested run that adds the assignee after the event fires does not cause the reviewer to remain assigned permanently - Give pull_request_review runs a reviewer-scoped concurrency key (reviewer-removal-{PR}-{login}) so concurrent review submissions from different reviewers are not cancelled by each other - Update removal-flow tests to populate state.currentPrData.assignees so the mock pulls.get returns the correct live assignee list; fix error-path mocks to return { data: { assignees: [...] } } Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
1 parent 3a07014 commit 86df148

2 files changed

Lines changed: 10 additions & 3 deletions

File tree

.github/scripts/__tests__/jest/bot-pr-add-reviewers-as-assignees.test.js

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,7 @@ describe('Bot: Add Reviewers as Assignees', () => {
251251

252252
test('removes reviewer from assignees when they submit a review', async () => {
253253
const state = createTestState();
254+
state.currentPrData = { assignees: [{ login: 'alice' }, { login: 'bob' }] };
254255
const ctx = createMockReviewContext({
255256
reviewerLogin: 'alice',
256257
assignees: [{ login: 'alice' }, { login: 'bob' }]
@@ -265,6 +266,7 @@ describe('Bot: Add Reviewers as Assignees', () => {
265266

266267
test('does not call removeAssignees when reviewer is not an assignee', async () => {
267268
const state = createTestState();
269+
state.currentPrData = { assignees: [{ login: 'alice' }] };
268270
const ctx = createMockReviewContext({
269271
reviewerLogin: 'carol',
270272
assignees: [{ login: 'alice' }]
@@ -278,6 +280,7 @@ describe('Bot: Add Reviewers as Assignees', () => {
278280

279281
test('review submitted by someone who was never a reviewer is a no-op', async () => {
280282
const state = createTestState();
283+
state.currentPrData = { assignees: [] };
281284
const ctx = createMockReviewContext({
282285
reviewerLogin: 'outsider',
283286
assignees: []
@@ -296,7 +299,7 @@ describe('Bot: Add Reviewers as Assignees', () => {
296299

297300
const errorMock = {
298301
rest: {
299-
pulls: { get: async () => ({}) },
302+
pulls: { get: async () => ({ data: { assignees: [{ login: 'alice' }] } }) },
300303
issues: {
301304
addAssignees: async () => {},
302305
removeAssignees: async () => {
@@ -319,7 +322,7 @@ describe('Bot: Add Reviewers as Assignees', () => {
319322

320323
const errorMock = {
321324
rest: {
322-
pulls: { get: async () => ({}) },
325+
pulls: { get: async () => ({ data: { assignees: [{ login: 'alice' }] } }) },
323326
issues: {
324327
addAssignees: async () => {},
325328
removeAssignees: async () => {

.github/scripts/bot-pr-add-reviewers-as-assignees.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,11 @@ async function removeReviewerFromAssignees({ github, context }) {
145145

146146
const owner = context.repo.owner;
147147
const repo = context.repo.repo;
148-
const currentAssignees = (pr.assignees || []).map(a => a.login);
148+
149+
// Fetch live PR data rather than relying on the event payload snapshot,
150+
// which may be stale if review_requested ran concurrently and added the assignee after this event fired.
151+
const livePr = (await github.rest.pulls.get({ owner, repo, pull_number: prNumber })).data;
152+
const currentAssignees = (livePr.assignees || []).map(a => a.login);
149153

150154
if (!currentAssignees.includes(reviewer)) {
151155
logger.log(`${reviewer} is not an assignee on PR #${prNumber}. Nothing to remove.`);

0 commit comments

Comments
 (0)