second attempt at handling double click accept/decline

kylehoehns · kylehoehns · commit 3c15d6e35e15 · 2025-10-03T17:55:39.000-05:00
diff --git a/CLAUDE.md b/CLAUDE.md
@@ -109,6 +109,18 @@ The project uses path aliases (configured in `tsconfig.json`):
 - `@utils/log` is mocked globally via `moduleNameMapper`
 - Timezone set to `America/Chicago` for tests
 
+## Development Workflow
+
+**IMPORTANT**: Before considering any task complete, ALWAYS run `pnpm verify` to ensure:
+
+- Linting passes
+- Code formatting is correct
+- TypeScript compilation succeeds
+- Build completes successfully
+- All tests pass
+
+This helps catch issues early and maintains code quality.
+
 ## Commit Conventions
 
 This repo uses [Conventional Commits](https://www.conventionalcommits.org/):
diff --git a/src/bot/__tests__/acceptReviewRequest.test.ts b/src/bot/__tests__/acceptReviewRequest.test.ts
@@ -63,14 +63,9 @@ jest.mock('@/services/HackParserService', () => ({
 
 describe('acceptReviewRequest', () => {
   beforeEach(() => {
-    activeReviewRepo.getReviewByThreadIdOrFail = jest.fn(
-      async () =>
-        ({
-          pdfIdentifier: 'example.pdf',
-          acceptedReviewers: [],
-          declinedReviewers: [],
-        }) as unknown as ActiveReview,
-    );
+    // Reset the mock to default implementation
+    // Note: Individual tests can override this mock as needed
+    activeReviewRepo.getReviewByThreadIdOrFail = jest.fn();
   });
 
   const expectedHackParserPDFBlock = {
@@ -129,6 +124,14 @@ describe('acceptReviewRequest', () => {
     ];
     action.body.message!.ts = '1234';
 
+    // Mock review with user in pending list
+    (activeReviewRepo.getReviewByThreadIdOrFail as jest.Mock).mockResolvedValue({
+      pdfIdentifier: 'example.pdf',
+      acceptedReviewers: [],
+      declinedReviewers: [],
+      pendingReviewers: [{ userId: action.body.user.id }],
+    } as unknown as ActiveReview);
+
     userRepo.markNowAsLastReviewedDate = resolve();
     chatService.replyToReviewThread = resolve();
     chatService.updateDirectMessage = resolve();
@@ -202,25 +205,25 @@ describe('acceptReviewRequest', () => {
         },
       ];
 
-      (activeReviewRepo.getReviewByThreadIdOrFail as jest.Mock).mockResolvedValueOnce({
+      // Mock review where user already accepted (not in pending)
+      (activeReviewRepo.getReviewByThreadIdOrFail as jest.Mock).mockResolvedValue({
         pdfIdentifier: 'example.pdf',
         acceptedReviewers: [{ userId }],
         declinedReviewers: [],
+        pendingReviewers: [],
       } as unknown as ActiveReview);
 
       const mockUpdateDirectMessage = jest.fn().mockReturnValue(() => Promise.resolve());
-      const mockAddUser = jest.fn().mockReturnValue(() => Promise.resolve());
       const mockMarkNow = jest.fn().mockReturnValue(() => Promise.resolve());
 
       chatService.updateDirectMessage = mockUpdateDirectMessage;
-      (addUserToAcceptedReviewers as jest.Mock).mockImplementation(mockAddUser);
       userRepo.markNowAsLastReviewedDate = mockMarkNow;
 
       const app = buildMockApp();
       acceptReviewRequest.setup(app);
       await acceptReviewRequest.handleAccept(action);
 
-      expect(mockAddUser).not.toHaveBeenCalled();
+      expect(addUserToAcceptedReviewers).not.toHaveBeenCalled();
       expect(mockMarkNow).not.toHaveBeenCalled();
       expect(mockUpdateDirectMessage).not.toHaveBeenCalled();
     });
@@ -242,25 +245,25 @@ describe('acceptReviewRequest', () => {
         },
       ];
 
-      (activeReviewRepo.getReviewByThreadIdOrFail as jest.Mock).mockResolvedValueOnce({
+      // Mock review where user already declined (not in pending)
+      (activeReviewRepo.getReviewByThreadIdOrFail as jest.Mock).mockResolvedValue({
         pdfIdentifier: 'example.pdf',
         acceptedReviewers: [],
         declinedReviewers: [{ userId }],
+        pendingReviewers: [],
       } as unknown as ActiveReview);
 
       const mockUpdateDirectMessage = jest.fn().mockReturnValue(() => Promise.resolve());
-      const mockAddUser = jest.fn().mockReturnValue(() => Promise.resolve());
       const mockMarkNow = jest.fn().mockReturnValue(() => Promise.resolve());
 
       chatService.updateDirectMessage = mockUpdateDirectMessage;
-      (addUserToAcceptedReviewers as jest.Mock).mockImplementation(mockAddUser);
       userRepo.markNowAsLastReviewedDate = mockMarkNow;
 
       const app = buildMockApp();
       acceptReviewRequest.setup(app);
       await acceptReviewRequest.handleAccept(action);
 
-      expect(mockAddUser).not.toHaveBeenCalled();
+      expect(addUserToAcceptedReviewers).not.toHaveBeenCalled();
       expect(mockMarkNow).not.toHaveBeenCalled();
       expect(mockUpdateDirectMessage).not.toHaveBeenCalled();
     });
@@ -278,10 +281,11 @@ describe('acceptReviewRequest', () => {
   });
 
   it('should work where there is no PDF identifier', async () => {
-    (activeReviewRepo.getReviewByThreadIdOrFail as jest.Mock).mockResolvedValueOnce({
+    (activeReviewRepo.getReviewByThreadIdOrFail as jest.Mock).mockResolvedValue({
       pdfIdentifier: '',
       acceptedReviewers: [],
       declinedReviewers: [],
+      pendingReviewers: [{ userId: 'test-user-id' }],
     } as unknown as ActiveReview);
 
     const { action } = await callHandleAccept();
diff --git a/src/bot/__tests__/declineReviewRequest.test.ts b/src/bot/__tests__/declineReviewRequest.test.ts
@@ -43,6 +43,7 @@ describe('declineReviewRequest', () => {
       const activeReview = {
         acceptedReviewers: [],
         declinedReviewers: [],
+        pendingReviewers: [{ userId: action.body.user.id }],
       };
 
       activeReviewRepo.getReviewByThreadIdOrFail = jest.fn().mockResolvedValueOnce(activeReview);
@@ -78,6 +79,7 @@ describe('declineReviewRequest', () => {
       const activeReview = {
         acceptedReviewers: [{ userId }],
         declinedReviewers: [],
+        pendingReviewers: [], // User already accepted, so not in pending
       };
 
       activeReviewRepo.getReviewByThreadIdOrFail = jest.fn().mockResolvedValueOnce(activeReview);
@@ -87,8 +89,8 @@ describe('declineReviewRequest', () => {
       declineReviewRequest.setup(app);
       await declineReviewRequest.handleDecline(action);
 
+      // Should not call decline because user is not in pending list
       expect(RequestService.declineRequest).not.toHaveBeenCalled();
-      expect(chatService.updateDirectMessage).not.toHaveBeenCalled();
     });
 
     it('should ignore duplicate decline clicks when user already declined', async () => {
@@ -112,6 +114,7 @@ describe('declineReviewRequest', () => {
       const activeReview = {
         acceptedReviewers: [],
         declinedReviewers: [{ userId }],
+        pendingReviewers: [], // User already declined, so not in pending
       };
 
       activeReviewRepo.getReviewByThreadIdOrFail = jest.fn().mockResolvedValueOnce(activeReview);
@@ -121,8 +124,8 @@ describe('declineReviewRequest', () => {
       declineReviewRequest.setup(app);
       await declineReviewRequest.handleDecline(action);
 
+      // Should not call decline because user is not in pending list
       expect(RequestService.declineRequest).not.toHaveBeenCalled();
-      expect(chatService.updateDirectMessage).not.toHaveBeenCalled();
     });
   });
 });
diff --git a/src/bot/acceptReviewRequest.ts b/src/bot/acceptReviewRequest.ts
@@ -38,19 +38,30 @@ export const acceptReviewRequest = {
 
       log.d('acceptReviewRequest.handleAccept', `${user.name} accepted review ${threadId}`);
 
-      // Idempotency check: If user already responded to this review, ignore duplicate clicks
+      // Quick check: If user already responded, ignore duplicate clicks
       const existingReview = await activeReviewRepo.getReviewByThreadIdOrFail(threadId);
-      const alreadyAccepted = existingReview.acceptedReviewers.some(r => r.userId === user.id);
-      const alreadyDeclined = existingReview.declinedReviewers.some(r => r.userId === user.id);
+      const isPending = existingReview.pendingReviewers.some(r => r.userId === user.id);
 
-      if (alreadyAccepted || alreadyDeclined) {
+      if (!isPending) {
         log.d(
           'acceptReviewRequest.handleAccept',
           `User ${user.id} already responded to review ${threadId}, ignoring duplicate click`,
         );
         return;
       }
 
+      // Try to add user to accepted reviewers - this will throw if they're not in pending list
+      // (race condition protection in case of simultaneous clicks)
+      try {
+        await addUserToAcceptedReviewers(user.id, threadId);
+      } catch (err) {
+        log.d(
+          'acceptReviewRequest.handleAccept',
+          `User ${user.id} already responded to review ${threadId} (race condition), ignoring duplicate click`,
+        );
+        return;
+      }
+
       // remove accept/decline buttons from original message and update it
       const blocks = blockUtils.removeBlock(body, BlockId.REVIEWER_DM_BUTTONS);
       blocks.push(textBlock('You accepted this review.'));
@@ -82,8 +93,6 @@ export const acceptReviewRequest = {
 
       await chatService.updateDirectMessage(client, user.id, body.message.ts, blocks);
 
-      await addUserToAcceptedReviewers(user.id, threadId);
-
       await userRepo.markNowAsLastReviewedDate(user.id);
 
       await chatService.replyToReviewThread(
diff --git a/src/bot/declineReviewRequest.ts b/src/bot/declineReviewRequest.ts
@@ -22,23 +22,30 @@ export const declineReviewRequest = {
       const user = body.user;
       const threadId = body.actions[0].value;
 
-      const review = await activeReviewRepo.getReviewByThreadIdOrFail(threadId);
-
       log.d('declineReviewRequest.handleDecline', `${user.name} declined review ${threadId}`);
 
-      // Idempotency check: If user already responded to this review, ignore duplicate clicks
-      const alreadyAccepted = review.acceptedReviewers.some(r => r.userId === user.id);
-      const alreadyDeclined = review.declinedReviewers.some(r => r.userId === user.id);
+      const review = await activeReviewRepo.getReviewByThreadIdOrFail(threadId);
 
-      if (alreadyAccepted || alreadyDeclined) {
+      // Check if user is in pending list - if not, they already responded
+      const isPending = review.pendingReviewers.some(r => r.userId === user.id);
+      if (!isPending) {
         log.d(
           'declineReviewRequest.handleDecline',
           `User ${user.id} already responded to review ${threadId}, ignoring duplicate click`,
         );
         return;
       }
 
-      await declineRequest(this.app, review, user.id);
+      // declineRequest will throw if user is not in pending (race condition protection)
+      try {
+        await declineRequest(this.app, review, user.id);
+      } catch (err) {
+        log.d(
+          'declineReviewRequest.handleDecline',
+          `User ${user.id} already responded to review ${threadId} (race condition), ignoring duplicate click`,
+        );
+        return;
+      }
 
       // eslint-disable-next-line @typescript-eslint/no-explicit-any
     } catch (err: any) {
diff --git a/src/services/RequestService.ts b/src/services/RequestService.ts
@@ -42,32 +42,36 @@ const closeRequestInternal = async (
   const priorPendingReviewer = updatedReview.pendingReviewers.find(
     ({ userId }) => userId === previousUserId,
   );
-  if (priorPendingReviewer) {
-    updatedReview.pendingReviewers = updatedReview.pendingReviewers.filter(
-      ({ userId }) => userId !== previousUserId,
-    );
-    updatedReview.declinedReviewers.push({
-      userId: previousUserId,
-      declinedAt: new Date().getTime(),
-    });
-    log.d(
-      'requestService.moveOnToNextPerson',
-      `Adding ${previousUserId} to declined reviewers for ${activeReview.threadId}`,
-    );
-    await activeReviewRepo.update(updatedReview);
-    const contextBlock = requestBuilder.buildReviewSectionBlock(
-      { id: updatedReview.requestorId },
-      updatedReview.languages,
-      DeadlineLabel.get(updatedReview.dueBy) || 'Unknown',
-    );
-    const closeMessageBlock = textBlock(closeMessage);
-    await chatService.updateDirectMessage(
-      app.client,
-      priorPendingReviewer.userId,
-      priorPendingReviewer.messageTimestamp,
-      [contextBlock, closeMessageBlock],
+  if (!priorPendingReviewer) {
+    throw new Error(
+      `${previousUserId} attempted to decline review ${activeReview.threadId} but was not in pending list`,
     );
   }
+
+  updatedReview.pendingReviewers = updatedReview.pendingReviewers.filter(
+    ({ userId }) => userId !== previousUserId,
+  );
+  updatedReview.declinedReviewers.push({
+    userId: previousUserId,
+    declinedAt: new Date().getTime(),
+  });
+  log.d(
+    'requestService.moveOnToNextPerson',
+    `Adding ${previousUserId} to declined reviewers for ${activeReview.threadId}`,
+  );
+  await activeReviewRepo.update(updatedReview);
+  const contextBlock = requestBuilder.buildReviewSectionBlock(
+    { id: updatedReview.requestorId },
+    updatedReview.languages,
+    DeadlineLabel.get(updatedReview.dueBy) || 'Unknown',
+  );
+  const closeMessageBlock = textBlock(closeMessage);
+  await chatService.updateDirectMessage(
+    app.client,
+    priorPendingReviewer.userId,
+    priorPendingReviewer.messageTimestamp,
+    [contextBlock, closeMessageBlock],
+  );
   return updatedReview;
 };
 
