fix: make the details dialog usable at speed — toasts and draw mode - #403
Merged
nGervasyuk merged 5 commits intoSep 1, 2026
Merged
nGervasyuk merged 5 commits into
nGervasyuk merged 5 commits into
Conversation
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.
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.
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.
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.
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.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five small fixes to the test run details dialog, split out of #402 so that one stays a pure performance change.
None of these are regressions from #402 — the toast anchor and the draw-mode leak both predate it. They surfaced because reviewing a screen went from about twenty seconds to about two, which turned rare annoyances into ones you meet in the first minute.
The toast landed on the buttons it was confirming
The app anchors toasts bottom-centre, which on the run list is the one strip 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 covered
Approve variations— the button needed for the next screen. With a five-second toast and a two-second screen, the reviewer had to detour to the dismiss cross before every single approval.Toasts raised from the dialog now anchor to the top, over the empty strip between the title and the icons. The list keeps the bottom anchor it was given in #390.
They stacked, then they never left
Approving screen after screen piled them up: the second and third arrived while the first was still there and pushed a green column down over the checkpoint header. A confirmation is interchangeable — the reviewer needs to know the last action went through, not keep a log — so a new one now closes the one before it.
Errors are deliberately excluded, with a test to keep them that way: a failed reject must not be swallowed by whatever the reviewer does next.
The duration is 2s rather than notistack's 5s, which was chosen for a page you stay on and outlived the screen the confirmation belonged to. Errors keep the default; they are worth reading and nothing is waiting on them.
Draw mode rode along to the next screen
The 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 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". This is where theYou have not saved ignore areasprompt came from, seemingly out of nowhere, after an approve or a reject.Draw mode and the selected rectangle now reset with the rest of the per-screen state.
A rejection read as a success
Rejecting raised the same green tick as approving, from buttons that sit next to each other and get pressed in a hurry. Both the single and the bulk rejection now use the neutral variant; approval keeps the green.
That required widening what counts as a confirmation for the replacement rule above — from "the success variant" to "anything that is not an error" — or the newly neutral rejections would have started stacking again.
Tests
Six Playwright tests in
testDetailsDialog.spec.ts, each watched failing first. They assert behaviour rather than snapshots: bounding boxes for the overlap, a count for the stacking, a bounded wait for the duration,aria-pressedafter navigation for draw mode, and the absence of the success class for the rejection.Worth knowing for anyone writing tests here: the
Tooltipwrapper makes itstitlethe accessible name of the button it wraps, soApproveis reachable asHotkey: Aand not by its label — these tests locate by text instead.Playwright 93/93, jest 26/26,
tscunchanged at the 30 pre-existing errors, eslint unchanged at its 6 pre-existing problems.