fix(ui): allow unreacting from comment popover

- fix selectors for hasReacted
- don't send empty HTML on reaction errors
- add E2E test

(cherry picked from commit b8a5ca2c40)

routers/web/repo/issue.go

@@ -3299,12 +3299,6 @@ func ChangeIssueReaction(ctx *context.Context) {
 			log.Info("CreateIssueReaction: %s", err)
 			break
 		}
-		// Reload new reactions
-		issue.Reactions = nil
-		if err = issue.LoadAttributes(ctx); err != nil {
-			log.Info("issue.LoadAttributes: %s", err)
-			break
-		}
 
 		log.Trace("Reaction for issue created: %d/%d/%d", ctx.Repo.Repository.ID, issue.ID, reaction.ID)
 	case "unreact":
@@ -3313,19 +3307,19 @@ func ChangeIssueReaction(ctx *context.Context) {
 			return
 		}
 
-		// Reload new reactions
-		issue.Reactions = nil
-		if err := issue.LoadAttributes(ctx); err != nil {
-			log.Info("issue.LoadAttributes: %s", err)
-			break
-		}
-
 		log.Trace("Reaction for issue removed: %d/%d", ctx.Repo.Repository.ID, issue.ID)
 	default:
 		ctx.NotFound(fmt.Sprintf("Unknown action %s", ctx.Params(":action")), nil)
 		return
 	}
 
+	// Reload new reactions
+	issue.Reactions = nil
+	if err := issue.LoadAttributes(ctx); err != nil {
+		ctx.ServerError("ChangeIssueReaction.LoadAttributes", err)
+		return
+	}
+
 	if len(issue.Reactions) == 0 {
 		ctx.JSON(http.StatusOK, map[string]any{
 			"empty": true,
@@ -3406,12 +3400,6 @@ func ChangeCommentReaction(ctx *context.Context) {
 			log.Info("CreateCommentReaction: %s", err)
 			break
 		}
-		// Reload new reactions
-		comment.Reactions = nil
-		if err = comment.LoadReactions(ctx, ctx.Repo.Repository); err != nil {
-			log.Info("comment.LoadReactions: %s", err)
-			break
-		}
 
 		log.Trace("Reaction for comment created: %d/%d/%d/%d", ctx.Repo.Repository.ID, comment.Issue.ID, comment.ID, reaction.ID)
 	case "unreact":
@@ -3420,19 +3408,19 @@ func ChangeCommentReaction(ctx *context.Context) {
 			return
 		}
 
-		// Reload new reactions
-		comment.Reactions = nil
-		if err = comment.LoadReactions(ctx, ctx.Repo.Repository); err != nil {
-			log.Info("comment.LoadReactions: %s", err)
-			break
-		}
-
 		log.Trace("Reaction for comment removed: %d/%d/%d", ctx.Repo.Repository.ID, comment.Issue.ID, comment.ID)
 	default:
 		ctx.NotFound(fmt.Sprintf("Unknown action %s", ctx.Params(":action")), nil)
 		return
 	}
 
+	// Reload new reactions
+	comment.Reactions = nil
+	if err = comment.LoadReactions(ctx, ctx.Repo.Repository); err != nil {
+		ctx.ServerError("ChangeCommentReaction.LoadReactions", err)
+		return
+	}
+
 	if len(comment.Reactions) == 0 {
 		ctx.JSON(http.StatusOK, map[string]any{
 			"empty": true,

tests/e2e/reaction-selectors.test.e2e.js

@@ -0,0 +1,65 @@
+// @ts-check
+import {test, expect} from '@playwright/test';
+import {login_user, load_logged_in_context} from './utils_e2e.js';
+
+test.beforeAll(async ({browser}, workerInfo) => {
+  await login_user(browser, workerInfo, 'user2');
+});
+
+const assertReactionCounts = (comment, counts) =>
+  expect(async () => {
+    await expect(comment.locator('.reactions')).toBeVisible();
+
+    const reactions = Object.fromEntries(
+      await Promise.all(
+        (
+          await comment
+            .locator(`.reactions [role=button][data-reaction-content]`)
+            .all()
+        ).map(async (button) => [
+          await button.getAttribute('data-reaction-content'),
+          parseInt(await button.locator('.reaction-count').textContent()),
+        ]),
+      ),
+    );
+    return expect(reactions).toStrictEqual(counts);
+  }).toPass();
+
+async function toggleReaction(menu, reaction) {
+  await menu.evaluateAll((menus) => menus[0].focus());
+  await menu.locator('.add-reaction').click();
+  await menu.locator(`[role=menuitem][data-reaction-content="${reaction}"]`).click();
+}
+
+test('Reaction Selectors', async ({browser}, workerInfo) => {
+  const context = await load_logged_in_context(browser, workerInfo, 'user2');
+  const page = await context.newPage();
+
+  const response = await page.goto('/user2/repo1/issues/1');
+  await expect(response?.status()).toBe(200);
+
+  const comment = page.locator('.comment#issuecomment-2').first();
+
+  const topPicker = comment.locator('.actions [role=menu].select-reaction');
+  const bottomPicker = comment.locator('.reactions').getByRole('menu');
+
+  await assertReactionCounts(comment, {'laugh': 2});
+
+  await toggleReaction(topPicker, '+1');
+  await assertReactionCounts(comment, {'laugh': 2, '+1': 1});
+
+  await toggleReaction(bottomPicker, '+1');
+  await assertReactionCounts(comment, {'laugh': 2});
+
+  await toggleReaction(bottomPicker, '-1');
+  await assertReactionCounts(comment, {'laugh': 2, '-1': 1});
+
+  await toggleReaction(topPicker, '-1');
+  await assertReactionCounts(comment, {'laugh': 2});
+
+  await comment.locator('.reactions [role=button][data-reaction-content=laugh]').click();
+  await assertReactionCounts(comment, {'laugh': 1});
+
+  await toggleReaction(topPicker, 'laugh');
+  await assertReactionCounts(comment, {'laugh': 2});
+});

web_src/js/features/comp/ReactionSelector.js

@@ -9,7 +9,7 @@ export function initCompReactionSelector($parent) {
 
     const actionUrl = this.closest('[data-action-url]')?.getAttribute('data-action-url');
     const reactionContent = this.getAttribute('data-reaction-content');
-    const hasReacted = this.closest('.ui.segment.reactions')?.querySelector(`a[data-reaction-content="${reactionContent}"]`)?.getAttribute('data-has-reacted') === 'true';
+    const hasReacted = this.closest('.comment')?.querySelector(`.ui.segment.reactions a[data-reaction-content="${reactionContent}"]`)?.getAttribute('data-has-reacted') === 'true';
 
     const res = await POST(`${actionUrl}/${hasReacted ? 'unreact' : 'react'}`, {
       data: new URLSearchParams({content: reactionContent}),