Accept image from tree

[sonic16x]

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/html-reporter@776

commit: 8ecfbe8

[github-actions]

✅ Component tests succeed

Report

[github-actions]

❌ E2E tests failed

Report

[shadowusr]

I know this wasn't introduced by this PR, but let's do something about box-shadow interfering with other text in the tree item, because with this new layout the issue becomes even more apparent:

Let's either remove the box-shadow completely or make it much smaller. It was originally introduced to improve the appearance of buttons displayed over an item’s text, so you can check that case to see which option looks better.

[shadowusr]

The accept button isn't centered relative to anything:

I think it would look better if it was centered relative to the "screenshots" area.

There's another interesting case: when accepting new image in tree-full-screen mode, the button is still at the center, but it's far away from the screenshot that we are actually dealing with:

I suggest to think about this case. IDK, one way of solving this would be to position accept button to the bottom-center of the Actual screenshot, not to the whole block. Or just live with how it is now, not sure.

[shadowusr]

Hmmm, test steps lost red background as well, while args are still red... This looks a bit weird IMO. I don't thing red background in test steps is a distracting factor and didn't think deeply about this area, so perhaps it's better to keep it as it was — with red bg?

[shadowusr]

A minor issue with diff bubbles in tree. While bubbles are active the whole "hover" state disappears as well as "accept" button. I think throwing pointer-events none on those bubbles should do the trick...

[shadowusr]

Seems like a case when tree takes minimal amount of space is worsened with the new design:

Before, we had 1 image preview in this narrow state. Now we have multiple elements overflowing tree — both in the toolbar and the screenshots as well. I think we should address both issues.

Probably it still makes sense to have some threshold and when tree is smaller in width than that to display only diff image and probably disable the modes selector in toolbar

[shadowusr]

Let's make this gap a tiny bit larger:

It's currently smaller than any other gap; gaps between "groups of elements" usually should be larger than gaps within "groups of elements"

[shadowusr]

Let's make border-radius of the accept button default 6px to match other buttons. I understand that 8px come from my figma — sorry, my bad!! I didn't use standard components in figma and to the eye 8px looked better, but consistency is more important, we shouldn't have buttons with different border-radius values on one page. Same goes for button text

Can we just reuse the "accept" button from test side?

[shadowusr]

We also discussed that these buttons should be "sticky" in a sense that when screenshots is partially off-screen, the accept button should still be visible and stick to bottom of the tree block. But that's optional, if it's too hard to implement, we can skip this for now.

[shadowusr]

One other tiny UX enhancement that we've talked about was that hovering over "accept button" should highlight the actual screenshot and you can click on actual screenshot to accept it (except for "switch"/"swipe"/"diff only" modes). While not mandatory, i think this would make accepting screenshots MUCH easier, because you don't have to find the button each time.

[shadowusr]

There's a hotkeys conflict currently:

Two accept buttons and both show "A" hotkey and which one will work is not clear.

Overall, "A" hotkeys in tree don't seem to work at all.

[shadowusr]

I am seeing the whole tree block crashing:

Error:

Error
Error: Rendered fewer hooks than expected. This may be caused by an accidental early return statement.
    at renderWithHooks (webpack-internal:///../../node_modules/react-dom/cjs/react-dom.development.js:15562:11)
    at updateFunctionComponent (webpack-internal:///../../node_modules/react-dom/cjs/react-dom.development.js:19612:20)
    at beginWork (webpack-internal:///../../node_modules/react-dom/cjs/react-dom.development.js:21635:16)
    at beginWork$1 (webpack-internal:///../../node_modules/react-dom/cjs/react-dom.development.js:27460:14)
    at performUnitOfWork (webpack-internal:///../../node_modules/react-dom/cjs/react-dom.development.js:26591:12)
    at workLoopSync (webpack-internal:///../../node_modules/react-dom/cjs/react-dom.development.js:26500:5)
    at renderRootSync (webpack-internal:///../../node_modules/react-dom/cjs/react-dom.development.js:26468:7)
    at recoverFromConcurrentError (webpack-internal:///../../node_modules/react-dom/cjs/react-dom.development.js:25884:20)
    at performConcurrentWorkOnRoot (webpack-internal:///../../node_modules/react-dom/cjs/react-dom.development.js:25784:22)
    at workLoop (webpack-internal:///../../node_modules/scheduler/cjs/scheduler.development.js:266:34)

The crash happens when i hit "retry" button and the tests finishes running.

BTW the error rendering is a bit incorrect (right padding is missing), but that's up to you to fix it or not, techinically, it's unrelated to your changes.

[shadowusr]

Highlightning of hovered screenshots is barely visible and looks more like a rendering artifact than an intended border change. I would either remove it or make it really noticeable (I would make it more noticeable as we've discussed before).

To be clear: highlighting by changing border is fine, but let's make that border thicker/more noticeable in color

Also, we've discussed that only the "Actual" screenshots should have this hover effect, not all 3 screenshots and that clicking on actual would accept the screenshot. Did you decide that this was a bad idea? Let's discuss. I think it would make it quite a bit more convenient for users.

[shadowusr]

If we got rid of backgrounds in the tree view, we should disable the logic that "stitched" together neighboring tree items. Look at this case:

Those square border radiuses now look weird.

[shadowusr]

You can still see the glow from the buttons intefering with the text on the left:

IMO the glow should only be visible in case of an actual overlap, like this one:

But if there's no overlap, the glow shouldn't affect anything. I would just drastically reduce its spread radius.

[shadowusr]

Buttons still overflow the tree card on the lowest width:

We should either increase the min allowed width or allow buttons to take white space in the middle. To the eye, there's more than enough space to fit everything there, but for some reason we choose to have a gap in the middle.

[shadowusr]

IMO screenshots should prevent click event propagation. Currently, clicking on "Accept" button in tree or trying to interact with screenshot unconditionally selects that tree item, which leads to scroll jump & confusion:

Screen.Recording.2026-08-04.at.3.39.40.PM.mov

This change would make it harder to select items (user may no longer just click on any part of the tree item, but current behavior is extremely confusing especially in the "full screen tree" mode, where it's not even clear what selecting an item does. So we should definitely handle this case better.

[shadowusr]

Also, I've noticed a bug with the new tree mode switcher. Reproduction steps:

1. Manually slide the tree to the full screen
2. Try to change the view via modes switcher

It will only switch between "full screen tree" and "full screen test", completely missing the "tree and test" option.

[shadowusr]

When clicking over here:

Nothing happens, why? I would expect it to open test in the right side. But the click is just ignored.

[shadowusr]

The switch mode seems to be broken, clicking on the image in Switch mode doesn't seem to do anything:

Was it broken before or did it break now?

[shadowusr]

There are few important UX flaws in the current implementation:

• After clicking accept or using a hotkey, there are severe jumps in scroll, size, and everything, which makes it
• When clicking "Accept" button, it still leads to selection of that item. It shouldn't. The click should only affect the button.
• If filter is set to failed screenshots, after accepting, it just disappears
• "fit to screen" mode doesn't actually fit it to screen, especially with new screenshots

Look at how it works in practice in new ui:

tree-ux.mp4

And this is how it was in old ui:

old-tree-ux.mp4

IMO the old UI is drastically better, the new ui is basically unusable for this scenario.

[shadowusr]

I personally strongly dislike fields that may have 3 different types: react element, boolean and undefined! The naming is misleading too, IMO. I took me a while to understand that this is actually a ref to scroll container, not actual magnifier. So why not call it scrollContainerRef and let it accept react object or be absent? I think it would be much easier to reason about in this case.