From 78676b41a018fa08de0d6703a7e9e6b34d5659ee Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Tue, 1 Sep 2026 10:02:45 +0300 Subject: [PATCH 1/5] fix: keep the dialog's toasts off its own approve buttons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The app anchors toasts bottom-centre because that is the one strip of the test run list clear of the build pagination on the left and the run pagination on the right. The details dialog is fullscreen and puts its approve/reject bar in exactly that spot, so every toast it raised landed on those buttons. That was survivable while a screen took twenty seconds to review. Now that it takes about two, a five-second toast covers "Approve variations" for the next screen and the one after it, and the reviewer has to go and click the dismiss cross before every approval — visible in the recording as a detour to the toast between each screen. Toasts raised from the dialog now anchor to the top, over the empty strip between its title and its icons; the list keeps the bottom anchor it was given. A test pins the two apart by their bounding boxes rather than by a snapshot, so it fails on the overlap itself rather than on a repaint. Locating the button there is worth a note: the tooltip wrapper makes "Hotkey: A" the accessible name, so the test matches on text instead. --- .../test/testDetailsDialog.spec.ts | 59 +++++++++++++++++++ .../ApproveRejectButtons.tsx | 4 +- .../MatchingVariationsDialog.tsx | 4 +- .../TestDetailsDialog/TestDetailsModal.tsx | 4 +- .../TestDetailsDialog/useDialogSnackbar.ts | 29 +++++++++ 5 files changed, 94 insertions(+), 6 deletions(-) create mode 100644 integration_tests/test/testDetailsDialog.spec.ts create mode 100644 src/components/TestDetailsDialog/useDialogSnackbar.ts diff --git a/integration_tests/test/testDetailsDialog.spec.ts b/integration_tests/test/testDetailsDialog.spec.ts new file mode 100644 index 00000000..7513dbb3 --- /dev/null +++ b/integration_tests/test/testDetailsDialog.spec.ts @@ -0,0 +1,59 @@ +import { expect } from "@playwright/test"; +import { test } from "fixtures"; +import { + TEST_BUILD_FAILED, + TEST_PROJECT, + 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]); + await mockTestRun(page, TEST_UNRESOLVED); + 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); +}); diff --git a/src/components/TestDetailsDialog/ApproveRejectButtons.tsx b/src/components/TestDetailsDialog/ApproveRejectButtons.tsx index a222b662..4c0f692b 100644 --- a/src/components/TestDetailsDialog/ApproveRejectButtons.tsx +++ b/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(); diff --git a/src/components/TestDetailsDialog/MatchingVariationsDialog.tsx b/src/components/TestDetailsDialog/MatchingVariationsDialog.tsx index e43151de..3b2f1b87 100644 --- a/src/components/TestDetailsDialog/MatchingVariationsDialog.tsx +++ b/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(); diff --git a/src/components/TestDetailsDialog/TestDetailsModal.tsx b/src/components/TestDetailsDialog/TestDetailsModal.tsx index e614ee5c..d46c1bd0 100644 --- a/src/components/TestDetailsDialog/TestDetailsModal.tsx +++ b/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 = ({ 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); diff --git a/src/components/TestDetailsDialog/useDialogSnackbar.ts b/src/components/TestDetailsDialog/useDialogSnackbar.ts new file mode 100644 index 00000000..0045d36b --- /dev/null +++ b/src/components/TestDetailsDialog/useDialogSnackbar.ts @@ -0,0 +1,29 @@ +import { useCallback } from "react"; +import { OptionsObject, SnackbarMessage, useSnackbar } from "notistack"; + +/** + * `useSnackbar`, but anchored to the top. + * + * 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 lands on the + * buttons — and since a toast lives five seconds while a screen now takes + * about two to review, "Approve variations" stayed covered until the reviewer + * dismissed it by hand, every single screen. + * + * Callers may still override the anchor per message. + */ +export const useDialogSnackbar = () => { + const { enqueueSnackbar, closeSnackbar } = useSnackbar(); + + const enqueue = useCallback( + (message: SnackbarMessage, options?: OptionsObject) => + enqueueSnackbar(message, { + anchorOrigin: { vertical: "top", horizontal: "center" }, + ...options, + }), + [enqueueSnackbar], + ); + + return { enqueueSnackbar: enqueue, closeSnackbar }; +}; From 906a9ba32ce22e82262f0789f601d811ee8aab9b Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Tue, 1 Sep 2026 10:44:09 +0300 Subject: [PATCH 2/5] fix: show one approval confirmation at a time, not a growing stack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Approving screen after screen piled the toasts up. A screen now takes a couple of seconds to review and a toast lives five, so the second and third confirmation arrived while the first was still up and pushed a green column down over the checkpoint's header. A confirmation is interchangeable — the reviewer only needs to know the last action went through — so a new one now closes the one before it. Errors are deliberately left out of that: they are not interchangeable, and a failed reject must not be swallowed by whatever the reviewer does next. A test pins that down alongside the stacking one, so a later tidy-up cannot quietly widen the replacement to cover errors too. --- .../test/testDetailsDialog.spec.ts | 41 +++++++++++++++++ .../TestDetailsDialog/useDialogSnackbar.ts | 46 ++++++++++++++----- 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/integration_tests/test/testDetailsDialog.spec.ts b/integration_tests/test/testDetailsDialog.spec.ts index 7513dbb3..0c47a3dd 100644 --- a/integration_tests/test/testDetailsDialog.spec.ts +++ b/integration_tests/test/testDetailsDialog.spec.ts @@ -57,3 +57,44 @@ test("raises its toast clear of the approve buttons", async ({ 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); +}); + +// 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(); +}); diff --git a/src/components/TestDetailsDialog/useDialogSnackbar.ts b/src/components/TestDetailsDialog/useDialogSnackbar.ts index 0045d36b..906bfd83 100644 --- a/src/components/TestDetailsDialog/useDialogSnackbar.ts +++ b/src/components/TestDetailsDialog/useDialogSnackbar.ts @@ -1,28 +1,52 @@ -import { useCallback } from "react"; -import { OptionsObject, SnackbarMessage, useSnackbar } from "notistack"; +import { useCallback, useRef } from "react"; +import { + OptionsObject, + SnackbarKey, + SnackbarMessage, + useSnackbar, +} from "notistack"; /** - * `useSnackbar`, but anchored to the top. + * `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 lands on the - * buttons — and since a toast lives five seconds while a screen now takes - * about two to review, "Approve variations" stayed covered until the reviewer - * dismissed it by hand, every single screen. + * 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(null); const enqueue = useCallback( - (message: SnackbarMessage, options?: OptionsObject) => - enqueueSnackbar(message, { + (message: SnackbarMessage, options?: OptionsObject) => { + const isConfirmation = options?.variant === "success"; + 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" }, ...options, - }), - [enqueueSnackbar], + }); + + if (isConfirmation) { + lastConfirmation.current = key; + } + return key; + }, + [enqueueSnackbar, closeSnackbar], ); return { enqueueSnackbar: enqueue, closeSnackbar }; From 707a2a6b1cfebfb9d2b5c4b75113af4030b956ab Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Tue, 1 Sep 2026 10:50:40 +0300 Subject: [PATCH 3/5] fix: let the approval confirmation go before the next screen is reviewed notistack's default five seconds was set for a page you stay on. A screen now takes a couple of seconds to review, so the confirmation outlived the screen it belonged to and, now that a new one replaces the last rather than stacking, simply sat over the checkpoint header for good. Confirmations from the dialog now last two seconds. Errors keep the default: they are worth reading, and no next screen is waiting on them. --- .../test/testDetailsDialog.spec.ts | 18 ++++++++++++++++++ .../TestDetailsDialog/useDialogSnackbar.ts | 4 ++++ 2 files changed, 22 insertions(+) diff --git a/integration_tests/test/testDetailsDialog.spec.ts b/integration_tests/test/testDetailsDialog.spec.ts index 0c47a3dd..3743c58d 100644 --- a/integration_tests/test/testDetailsDialog.spec.ts +++ b/integration_tests/test/testDetailsDialog.spec.ts @@ -74,6 +74,24 @@ test("replaces its confirmation rather than stacking them up", async ({ 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 ({ diff --git a/src/components/TestDetailsDialog/useDialogSnackbar.ts b/src/components/TestDetailsDialog/useDialogSnackbar.ts index 906bfd83..600096fb 100644 --- a/src/components/TestDetailsDialog/useDialogSnackbar.ts +++ b/src/components/TestDetailsDialog/useDialogSnackbar.ts @@ -38,6 +38,10 @@ export const useDialogSnackbar = () => { 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, }); From b02ae65fa6630e6d8efc38c8a8ffe426e0607827 Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Tue, 1 Sep 2026 10:58:03 +0300 Subject: [PATCH 4/5] fix: leave draw mode behind when moving to another run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The details dialog is not remounted as the reviewer steps between runs, so state belonging to one screen has to be cleared with it. The fade and the blend already were. Draw mode was not. Turning it on and moving on therefore carried it to the next screen, where the next click quietly drew an ignore area on a run the reviewer never meant to edit. That marked the run touched, and since the unsaved-changes guard runs before navigating while the flag is only cleared by navigating, the reviewer was stuck on that screen until they answered "discard" — the "You have not saved ignore areas" prompt appearing out of nowhere after an approve or reject. Draw mode and the selected rectangle now reset with the rest of the per-screen state. The bug predates this branch; nothing here caused it. It is fixed now because reviewing a screen went from twenty seconds to about two, which turned a trap you could go months without meeting into one you meet in the first few minutes. --- .../test/testDetailsDialog.spec.ts | 26 ++++++++++++++++++- .../TestDetailsDialog/TestDetailsModal.tsx | 11 ++++++-- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/integration_tests/test/testDetailsDialog.spec.ts b/integration_tests/test/testDetailsDialog.spec.ts index 3743c58d..ca53076f 100644 --- a/integration_tests/test/testDetailsDialog.spec.ts +++ b/integration_tests/test/testDetailsDialog.spec.ts @@ -3,6 +3,7 @@ import { test } from "fixtures"; import { TEST_BUILD_FAILED, TEST_PROJECT, + TEST_RUN_NEW, TEST_UNRESOLVED, } from "~client/_test/test.data.helper"; import { @@ -22,8 +23,9 @@ 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]); + 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"); @@ -116,3 +118,25 @@ test("lets an error outlive the confirmation that follows it", async ({ 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"); +}); diff --git a/src/components/TestDetailsDialog/TestDetailsModal.tsx b/src/components/TestDetailsDialog/TestDetailsModal.tsx index d46c1bd0..8d2f1343 100644 --- a/src/components/TestDetailsDialog/TestDetailsModal.tsx +++ b/src/components/TestDetailsDialog/TestDetailsModal.tsx @@ -234,12 +234,19 @@ const TestDetailsModal: React.FunctionComponent = ({ 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(() => { From 2a80608db05bda32862cba8fdf48248b03e6668f Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Tue, 1 Sep 2026 11:01:51 +0300 Subject: [PATCH 5/5] fix: stop a rejection reading as a success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rejecting raised the same green tick as approving, in the same place, with the two buttons sitting next to each other and pressed in a hurry. At a glance there was nothing to tell the two outcomes apart. Both the single reject and the bulk one now use the neutral variant; the approval keeps the green. That meant widening what the dialog treats as a confirmation for the purpose of replacing the last toast: not "the success variant" but "anything that is not an error". An approval and a rejection are both just the last action going through, and neither needs to outlive the other on screen — without this, the neutral rejections would have started stacking up again. --- .../test/testDetailsDialog.spec.ts | 24 +++++++++++++++++++ .../ApproveRejectButtons.tsx | 5 +++- .../MatchingVariationsDialog.tsx | 4 +++- .../TestDetailsDialog/useDialogSnackbar.ts | 5 +++- 4 files changed, 35 insertions(+), 3 deletions(-) diff --git a/integration_tests/test/testDetailsDialog.spec.ts b/integration_tests/test/testDetailsDialog.spec.ts index ca53076f..1b362a94 100644 --- a/integration_tests/test/testDetailsDialog.spec.ts +++ b/integration_tests/test/testDetailsDialog.spec.ts @@ -140,3 +140,27 @@ test("leaves draw mode behind when moving to another run", async ({ 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/); +}); diff --git a/src/components/TestDetailsDialog/ApproveRejectButtons.tsx b/src/components/TestDetailsDialog/ApproveRejectButtons.tsx index 4c0f692b..ffef0b07 100644 --- a/src/components/TestDetailsDialog/ApproveRejectButtons.tsx +++ b/src/components/TestDetailsDialog/ApproveRejectButtons.tsx @@ -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(); }) diff --git a/src/components/TestDetailsDialog/MatchingVariationsDialog.tsx b/src/components/TestDetailsDialog/MatchingVariationsDialog.tsx index 3b2f1b87..b901c212 100644 --- a/src/components/TestDetailsDialog/MatchingVariationsDialog.tsx +++ b/src/components/TestDetailsDialog/MatchingVariationsDialog.tsx @@ -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", }, ); }) diff --git a/src/components/TestDetailsDialog/useDialogSnackbar.ts b/src/components/TestDetailsDialog/useDialogSnackbar.ts index 600096fb..e9626212 100644 --- a/src/components/TestDetailsDialog/useDialogSnackbar.ts +++ b/src/components/TestDetailsDialog/useDialogSnackbar.ts @@ -30,7 +30,10 @@ export const useDialogSnackbar = () => { const enqueue = useCallback( (message: SnackbarMessage, options?: OptionsObject) => { - const isConfirmation = options?.variant === "success"; + // 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);