fix: make the details dialog usable at speed — toasts and draw mode

integration_tests/test/testDetailsDialog.spec.ts

@@ -0,0 +1,166 @@
+import { expect } from "@playwright/test";
+import { test } from "fixtures";
+import {
+  TEST_BUILD_FAILED,
+  TEST_PROJECT,
+  TEST_RUN_NEW,
+  TEST_UNRESOLVED,
+} from "~client/_test/test.data.helper";
+import {
+  API_URL,
+  mockGetBuildDetails,
+  mockGetBuilds,
+  mockGetProjects,
+  mockGetTestRuns,
+  mockImage,
+  mockTestRun,
+} from "utils/mocks";
+
+const project = TEST_PROJECT;
+const build = TEST_BUILD_FAILED;
+
+test.beforeEach(async ({ page }) => {
+  await mockGetProjects(page, [project]);
+  await mockGetBuilds(page, project.id, [build]);
+  await mockGetBuildDetails(page, build);
+  await mockGetTestRuns(page, build.id, [TEST_UNRESOLVED, TEST_RUN_NEW]);
+  await mockTestRun(page, TEST_UNRESOLVED);
+  await mockTestRun(page, TEST_RUN_NEW);
+  await mockImage(page, "image.png");
+  await mockImage(page, "diff.png");
+  await mockImage(page, "baseline.png");
+  await page.route(`${API_URL}/test-runs/approve?merge=false`, (route) =>
+    route.fulfill({ status: 200, body: "[]" }),
+  );
+});
+
+// The app anchors toasts bottom-centre, which is clear of both paginations on
+// the list. The dialog puts its approve/reject bar in exactly that spot, so a
+// toast there lands on the buttons and blocks the next screen's approval until
+// it times out or is dismissed.
+test("raises its toast clear of the approve buttons", async ({
+  openProjectPage,
+  page,
+}) => {
+  const projectPage = await openProjectPage(
+    project.id,
+    build.id,
+    TEST_UNRESOLVED.id,
+  );
+  // by text, not by role name: the tooltip wrapper makes "Hotkey: A" the
+  // button's accessible name. Anchored so "Approve variations" cannot match.
+  const approve = page.locator("button").filter({ hasText: /^Approve$/ });
+  await expect(approve).toBeVisible();
+  const buttons = await approve.boundingBox();
+
+  await approve.click();
+
+  await expect(projectPage.notification.message).toBeVisible();
+  const toast = await projectPage.notification.message.boundingBox();
+  expect(toast.y + toast.height).toBeLessThanOrEqual(buttons.y);
+});
+
+// A screen now takes a couple of seconds to review and a toast lives five, so
+// approving one after another piles them up over the checkpoint's header.
+test("replaces its confirmation rather than stacking them up", async ({
+  openProjectPage,
+  page,
+}) => {
+  await openProjectPage(project.id, build.id, TEST_UNRESOLVED.id);
+  const approve = page.locator("button").filter({ hasText: /^Approve$/ });
+
+  await approve.click();
+  await approve.click();
+  await approve.click();
+
+  await expect(page.getByText("Approved")).toHaveCount(1);
+});
+
+// notistack's default is five seconds, which outlives the screen the
+// confirmation belongs to and leaves one sitting over the header for good.
+test("lets the confirmation go before the next screen is reviewed", async ({
+  openProjectPage,
+  page,
+}) => {
+  await openProjectPage(project.id, build.id, TEST_UNRESOLVED.id);
+
+  await page
+    .locator("button")
+    .filter({ hasText: /^Approve$/ })
+    .click();
+
+  await expect(page.getByText("Approved")).toBeVisible();
+  // comfortably past a two-second toast, comfortably short of a five-second one
+  await expect(page.getByText("Approved")).toBeHidden({ timeout: 3500 });
+});
+
+// Errors are not interchangeable the way the confirmations are: a failure must
+// not be swallowed by whatever the reviewer does next.
+test("lets an error outlive the confirmation that follows it", async ({
+  openProjectPage,
+  page,
+}) => {
+  await page.route(`${API_URL}/test-runs/reject`, (route) =>
+    route.fulfill({ status: 500, body: JSON.stringify({ message: "nope" }) }),
+  );
+  await openProjectPage(project.id, build.id, TEST_UNRESOLVED.id);
+
+  await page
+    .locator("button")
+    .filter({ hasText: /^Reject$/ })
+    .click();
+  await expect(page.getByText("nope")).toBeVisible();
+  await page
+    .locator("button")
+    .filter({ hasText: /^Approve$/ })
+    .click();
+
+  await expect(page.getByText("nope")).toBeVisible();
+  await expect(page.getByText("Approved")).toBeVisible();
+});
+
+// The dialog stays mounted as the reviewer steps between runs, so state that
+// belongs to one screen has to be cleared with it. Draw mode was not: it rode
+// along, and the next click on a screenshot quietly drew an ignore area there,
+// marking a run touched that the reviewer never meant to edit — which then
+// blocked the very navigation that would have cleared the flag.
+test("leaves draw mode behind when moving to another run", async ({
+  openProjectPage,
+  page,
+}) => {
+  await openProjectPage(project.id, build.id, TEST_UNRESOLVED.id);
+  // the toggle is an icon with no label, so it is located by the value MUI
+  // puts on the button element
+  const drawMode = page.locator('button[value="drawMode"]');
+
+  await drawMode.click();
+  await expect(drawMode).toHaveAttribute("aria-pressed", "true");
+  await page.keyboard.press("ArrowRight");
+
+  await expect(page).toHaveURL(new RegExp(`testId=${TEST_RUN_NEW.id}$`));
+  await expect(drawMode).toHaveAttribute("aria-pressed", "false");
+});
+
+// A rejection is not a success. Dressing it in the same green tick as an
+// approval makes the two indistinguishable at a glance, which matters most
+// when they sit next to each other and are pressed in a hurry.
+test("does not dress a rejection up as a success", async ({
+  openProjectPage,
+  page,
+}) => {
+  await page.route(`${API_URL}/test-runs/reject`, (route) =>
+    route.fulfill({ status: 200, body: "[]" }),
+  );
+  await openProjectPage(project.id, build.id, TEST_UNRESOLVED.id);
+
+  await page
+    .locator("button")
+    .filter({ hasText: /^Reject$/ })
+    .click();
+
+  const toast = page.locator(".notistack-MuiContent", {
+    hasText: "Rejected",
+  });
+  await expect(toast).toBeVisible();
+  await expect(toast).not.toHaveClass(/notistack-MuiContent-success/);
+});

src/components/TestDetailsDialog/ApproveRejectButtons.tsx

@@ -1,5 +1,5 @@
 import { Chip, Button } from "@mui/material";
-import { useSnackbar } from "notistack";
+import { useDialogSnackbar } from "./useDialogSnackbar";
 import { useHotkeys } from "react-hotkeys-hook";
 import React from "react";
 import { testRunService } from "../../services";
@@ -32,7 +32,7 @@ export const ApproveRejectButtons: React.FunctionComponent<{
   afterReject?: () => void;
   onOpenVariations: (mode: MatchingVariationsMode) => void;
 }> = ({ testRun, afterApprove, afterReject, onOpenVariations }) => {
-  const { enqueueSnackbar } = useSnackbar();
+  const { enqueueSnackbar } = useDialogSnackbar();
   const classes = useStyles();
   const { selectedProjectId, projectList } = useProjectState();
   const { testRuns } = useTestRunState();
@@ -73,8 +73,11 @@ export const ApproveRejectButtons: React.FunctionComponent<{
     testRunService
       .rejectBulk([testRun.id])
       .then(() => {
+        // not a success: a rejection is a deliberate outcome, and the green
+        // tick made it read as an approval at a glance — the two buttons sit
+        // next to each other and get pressed in a hurry
         enqueueSnackbar("Rejected", {
-          variant: "success",
+          variant: "info",
         });
         afterReject && afterReject();
       })

src/components/TestDetailsDialog/MatchingVariationsDialog.tsx

@@ -23,7 +23,7 @@ import CloseIcon from "@mui/icons-material/Close";
 import ChevronLeftIcon from "@mui/icons-material/ChevronLeft";
 import ChevronRightIcon from "@mui/icons-material/ChevronRight";
 import { makeStyles } from "@mui/styles";
-import { useSnackbar } from "notistack";
+import { useDialogSnackbar } from "./useDialogSnackbar";
 import { useNavigate } from "react-router";
 import { Tooltip } from "../Tooltip";
 import { testRunService, staticService } from "../../services";
@@ -152,7 +152,7 @@ export const MatchingVariationsDialog: React.FunctionComponent<{
   onClose: () => void;
 }> = ({ mode, testRun, groupBy = "customTags", onClose }) => {
   const classes = useStyles();
-  const { enqueueSnackbar } = useSnackbar();
+  const { enqueueSnackbar } = useDialogSnackbar();
   const navigate = useNavigate();
   const { testRuns: allTestRuns, filteredSortedTestRunIds } = useTestRunState();
   const { selectedBuild } = useBuildState();
@@ -399,7 +399,9 @@ export const MatchingVariationsDialog: React.FunctionComponent<{
             ids.length
           } variations`,
           {
-            variant: "success",
+            // green for an approval only: a rejection is a deliberate outcome,
+            // not a success, and must not read as its opposite
+            variant: mode === "approve" ? "success" : "info",
           },
         );
       })

src/components/TestDetailsDialog/TestDetailsModal.tsx

@@ -47,7 +47,7 @@ import { routes, GO_TO_NEXT_KEY } from "../../constants";
 import { useTestRunDispatch, useProjectState } from "../../contexts";
 import { DrawArea, ImageStateLoad } from "./DrawArea";
 import { CommentsPopper } from "../CommentsPopper";
-import { useSnackbar } from "notistack";
+import { useDialogSnackbar } from "./useDialogSnackbar";
 import { ApproveRejectButtons } from "./ApproveRejectButtons";
 import {
   MatchingVariationsDialog,
@@ -137,7 +137,7 @@ const TestDetailsModal: React.FunctionComponent<TestDetailsModalProps> = ({
   handleClose,
 }) => {
   const classes = useStyles();
-  const { enqueueSnackbar } = useSnackbar();
+  const { enqueueSnackbar } = useDialogSnackbar();
   const testRunDispatch = useTestRunDispatch();
   const { selectedProjectId, projectList } = useProjectState();
   const project = projectList.find((item) => item.id === selectedProjectId);
@@ -234,12 +234,19 @@ const TestDetailsModal: React.FunctionComponent<TestDetailsModalProps> = ({
     resetPosition();
   };
 
-  // the fade and the blend belong to the run being looked at, like the diff
-  // does: a half-faded image carried onto the next screenshot would hide it
+  // the fade, the blend and the drawing tools belong to the run being looked
+  // at, like the diff does: a half-faded image carried onto the next
+  // screenshot would hide it, and draw mode carried over turned the reviewer's
+  // next click into an ignore area on a screen they never meant to edit —
+  // which marked that run touched and then blocked the very navigation that
+  // would have cleared the flag. This dialog is not remounted between runs, so
+  // whatever belongs to one screen has to be cleared here.
   useEffect(() => {
     setIsDiffShown(!!testRun.diffName);
     setOverlayOpacity(1);
     setBlendDifference(false);
+    setIsDrawMode(false);
+    setSelectedRectId(undefined);
   }, [testRun.id, testRun.diffName]);
 
   useEffect(() => {

src/components/TestDetailsDialog/useDialogSnackbar.ts

@@ -0,0 +1,60 @@
+import { useCallback, useRef } from "react";
+import {
+  OptionsObject,
+  SnackbarKey,
+  SnackbarMessage,
+  useSnackbar,
+} from "notistack";
+
+/**
+ * `useSnackbar` for the details dialog: anchored to the top, and showing one
+ * confirmation at a time.
+ *
+ * The app anchors toasts bottom-centre, which on the test run list is the one
+ * strip clear of both paginations. This dialog is fullscreen and puts its
+ * approve/reject bar in exactly that spot, so a toast raised here would land on
+ * the buttons the reviewer is about to press.
+ *
+ * Reviewing a screen now takes a couple of seconds and a toast lives five, so
+ * approving one after another piled them up over the checkpoint's header.
+ * Confirmations are interchangeable — the reviewer only needs to know the last
+ * action went through — so a new one takes the place of the one before it.
+ * Errors are never replaced: a failure must not be swallowed by whatever the
+ * reviewer happens to do next.
+ *
+ * Callers may still override the anchor per message.
+ */
+export const useDialogSnackbar = () => {
+  const { enqueueSnackbar, closeSnackbar } = useSnackbar();
+  const lastConfirmation = useRef<SnackbarKey | null>(null);
+
+  const enqueue = useCallback(
+    (message: SnackbarMessage, options?: OptionsObject) => {
+      // anything that is not an error: an approval and a rejection are both
+      // just "the last action went through", and neither needs to outlive the
+      // other on screen
+      const isConfirmation = options?.variant !== "error";
+      if (isConfirmation && lastConfirmation.current !== null) {
+        // a no-op once that toast has timed out on its own
+        closeSnackbar(lastConfirmation.current);
+      }
+
+      const key = enqueueSnackbar(message, {
+        anchorOrigin: { vertical: "top", horizontal: "center" },
+        // notistack's five seconds outlives the screen a confirmation belongs
+        // to, so one sat over the header permanently. Errors keep the default:
+        // they are worth reading, and there is no next screen waiting on them.
+        ...(isConfirmation ? { autoHideDuration: 2000 } : {}),
+        ...options,
+      });
+
+      if (isConfirmation) {
+        lastConfirmation.current = key;
+      }
+      return key;
+    },
+    [enqueueSnackbar, closeSnackbar],
+  );
+
+  return { enqueueSnackbar: enqueue, closeSnackbar };
+};