[Bug fix] Trace analytics scroll bar reset

[TackAdam]

Description

This change resets the scroll bar to the position it was at prior to opening the fly-out.

Issues Resolved

#1916

Check List

• New functionality includes testing.
  • All tests pass, including unit test, integration test and doctest
• New functionality has been documented.
  • New functionality has javadoc added
  • New functionality has user manual doc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[joshuali925]

lgtm

[ps48]

Question: why do we need setTimeout with delay 0 here?

[TackAdam]

Without it, closing the fly-out will reset the scroll bar. I believe this happens because the fly-out table closing behavior is handled in span_detail_table and it needs a delay for the use-effect.

[TackAdam]

This fix results in a slight flicker as the scroll bar adjust after being rendered to the correct position. I have not been able to find a fix that prevents this. Have tried using useMemo as it is handled in the SpanList menu but the click event for Timeline is happening in a different area.

[joshuali925]

scrollbar resetting on rerender is not normal behavior, it might be caused by the plot rerendering. if recovering scroll position flickers, you can try to memorize the plot component instead

[Swiddis]

I see that we rely on a memo for generating the span list and inject that value directly, but I wonder why we need a memo at all? Can we use the spans as JSX and let React do the rendering? I suspect that memo is where the reset is sneaking in to begin with.

[joshuali925]

i don't think the span list is relevant for this bug, since triggering the bug doesn't need user to go to the span list view, that component was never mounted

[Swiddis]

Ah, got confused between this being the span view as a whole and there being a timeline option under the span view. But the same thing applies to the timeline, both of them are being memoized.

[joshuali925]

the gantt chart is not memorized, memorizing it could workaround this bug. you can try adding this above return,

  const ganttChart = useMemo(
    () => (
      <Plt
        data={data.gantt}
        layout={layout}
        onClickHandler={onClick}
        onHoverHandler={onHover}
        onUnhoverHandler={onUnhover}
      />
    ),
    [data.gantt, layout]
  );

and change

dashboards-observability/public/components/trace_analytics/components/traces/span_detail_panel.tsx

Lines 296 to 306 in 4fa3705

	{toggleIdSelected === 'timeline' ? (
	<Plt
	data={data.gantt}
	layout={layout}
	onClickHandler={onClick}
	onHoverHandler={onHover}
	onUnhoverHandler={onUnhover}
	/>
	) : (
	spanDetailTable
	)}

to

{toggleIdSelected === 'timeline' ? ganttChart : spanDetailTable}

[TackAdam]

Implemented Josh's suggestion and tested. All working correctly.

[joshuali925]

for completeness and good practice, the dependency array should include all dependencies. so the correct way would be using useCallback to memorize the event handlers

  const onClick = useCallback(
    (event: any) => {
      if (!event?.points) return;
      const point = event.points[0];
      if (fromApp) {
        props.openSpanFlyout(point.data.spanId);
      } else {
        setCurrentSpan(point.data.spanId);
      }
    },
    [props.openSpanFlyout, setCurrentSpan, fromApp]
  );

  const onHover = useCallback(() => {
    const dragLayer = document.getElementsByClassName('nsewdrag')?.[0];
    dragLayer.style.cursor = 'pointer';
  }, []);

  const onUnhover = useCallback(() => {
    const dragLayer = document.getElementsByClassName('nsewdrag')?.[0];
    dragLayer.style.cursor = '';
  }, []);

and put them in the dependency array

  const ganttChart = useMemo(
    () => (
      <Plt
        data={data.gantt}
        layout={layout}
        onClickHandler={onClick}
        onHoverHandler={onHover}
        onUnhoverHandler={onUnhover}
      />
    ),
    [data.gantt, layout, onClick, onHover, onUnhover]
  );

some functions like setCurrentSpan would never change. but if something like fromApp changes, onClick will be re-created with the correct behavior. And since onClick changed and ganttChart depends on it, ganttChart component will also be re-created with the correct click behavior.

it likely won't make a real difference, since fromApp shouldn't change

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/dashboards-observability/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/dashboards-observability/backport-2.x
# Create a new branch
git switch --create backport/backport-1917-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 cc193f6665f0f6a87846cd15016346f030a32545
# Push it to GitHub
git push --set-upstream origin backport/backport-1917-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/dashboards-observability/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1917-to-2.x.