Logs: Fix permalinks not scrolling into view (#73464)

* fix log line scrolling

* add scrolling tests

* fix `LogRow` tests

public/app/features/explore/Logs/Logs.test.tsx

@@ -169,6 +169,79 @@ describe('Logs', () => {
     return render(getComponent(partialProps, logs));
   };
 
+  describe('scrolling behavior', () => {
+    let originalInnerHeight: number;
+    beforeEach(() => {
+      originalInnerHeight = window.innerHeight;
+      window.innerHeight = 1000;
+      window.HTMLElement.prototype.scrollIntoView = jest.fn();
+      window.HTMLElement.prototype.scroll = jest.fn();
+    });
+    afterEach(() => {
+      window.innerHeight = originalInnerHeight;
+    });
+
+    describe('when `exploreScrollableLogsContainer` is set', () => {
+      let featureToggle: boolean | undefined;
+      beforeEach(() => {
+        featureToggle = config.featureToggles.exploreScrollableLogsContainer;
+        config.featureToggles.exploreScrollableLogsContainer = true;
+      });
+      afterEach(() => {
+        config.featureToggles.exploreScrollableLogsContainer = featureToggle;
+        jest.clearAllMocks();
+      });
+
+      it('should call `this.state.logsContainer.scroll`', () => {
+        const scrollIntoViewSpy = jest.spyOn(window.HTMLElement.prototype, 'scrollIntoView');
+        jest.spyOn(window.HTMLElement.prototype, 'scrollTop', 'get').mockReturnValue(920);
+        const scrollSpy = jest.spyOn(window.HTMLElement.prototype, 'scroll');
+
+        const logs = [];
+        for (let i = 0; i < 50; i++) {
+          logs.push(makeLog({ uid: `uid${i}`, rowId: `id${i}`, timeEpochMs: i }));
+        }
+
+        setup({ panelState: { logs: { id: 'uid47' } } }, logs);
+
+        expect(scrollIntoViewSpy).toBeCalledTimes(1);
+        // element.getBoundingClientRect().top will always be 0 for jsdom
+        // calc will be `this.state.logsContainer.scrollTop - window.innerHeight / 2` -> 920 - 500 = 420
+        expect(scrollSpy).toBeCalledWith({ behavior: 'smooth', top: 420 });
+      });
+    });
+
+    describe('when `exploreScrollableLogsContainer` is not set', () => {
+      let featureToggle: boolean | undefined;
+      beforeEach(() => {
+        featureToggle = config.featureToggles.exploreScrollableLogsContainer;
+        config.featureToggles.exploreScrollableLogsContainer = false;
+      });
+      afterEach(() => {
+        config.featureToggles.exploreScrollableLogsContainer = featureToggle;
+      });
+
+      it('should call `scrollElement.scroll`', () => {
+        const logs = [];
+        for (let i = 0; i < 50; i++) {
+          logs.push(makeLog({ uid: `uid${i}`, rowId: `id${i}`, timeEpochMs: i }));
+        }
+        const scrollElementMock = {
+          scroll: jest.fn(),
+          scrollTop: 920,
+        };
+        setup(
+          { scrollElement: scrollElementMock as unknown as HTMLDivElement, panelState: { logs: { id: 'uid47' } } },
+          logs
+        );
+
+        // element.getBoundingClientRect().top will always be 0 for jsdom
+        // calc will be `scrollElement.scrollTop - window.innerHeight / 2` -> 920 - 500 = 420
+        expect(scrollElementMock.scroll).toBeCalledWith({ behavior: 'smooth', top: 420 });
+      });
+    });
+  });
+
   it('should render logs', () => {
     setup();
     const logsSection = screen.getByTestId('logRows');

public/app/features/explore/Logs/Logs.tsx

@@ -117,6 +117,7 @@ interface State {
   contextRow?: LogRowModel;
   tableFrame?: DataFrame;
   visualisationType?: VisualisationType;
+  logsContainer?: HTMLDivElement;
 }
 
 const scrollableLogsContainer = config.featureToggles.exploreScrollableLogsContainer;
@@ -144,7 +145,6 @@ class UnthemedLogs extends PureComponent<Props, State> {
   cancelFlippingTimer?: number;
   topLogsRef = createRef<HTMLDivElement>();
   logsVolumeEventBus: EventBus;
-  logsContainer = createRef<HTMLDivElement>();
 
   state: State = {
     showLabels: store.getBool(SETTINGS_KEYS.showLabels, false),
@@ -161,6 +161,7 @@ class UnthemedLogs extends PureComponent<Props, State> {
     contextRow: undefined,
     tableFrame: undefined,
     visualisationType: 'logs',
+    logsContainer: undefined,
   };
 
   constructor(props: Props) {
@@ -204,6 +205,10 @@ class UnthemedLogs extends PureComponent<Props, State> {
     }
   };
 
+  onLogsContainerRef = (node: HTMLDivElement) => {
+    this.setState({ logsContainer: node });
+  };
+
   onChangeLogsSortOrder = () => {
     this.setState({ isFlipping: true });
     // we are using setTimeout here to make sure that disabled button is rendered before the rendering of reordered logs
@@ -406,11 +411,11 @@ class UnthemedLogs extends PureComponent<Props, State> {
 
   scrollIntoView = (element: HTMLElement) => {
     if (config.featureToggles.exploreScrollableLogsContainer) {
-      this.scrollToTopLogs();
-      if (this.logsContainer.current) {
-        this.logsContainer.current.scroll({
+      if (this.state.logsContainer) {
+        this.topLogsRef.current?.scrollIntoView();
+        this.state.logsContainer.scroll({
           behavior: 'smooth',
-          top: this.logsContainer.current.scrollTop + element.getBoundingClientRect().top - window.innerHeight / 2,
+          top: this.state.logsContainer.scrollTop + element.getBoundingClientRect().top - window.innerHeight / 2,
         });
       }
 
@@ -456,8 +461,8 @@ class UnthemedLogs extends PureComponent<Props, State> {
 
   scrollToTopLogs = () => {
     if (config.featureToggles.exploreScrollableLogsContainer) {
-      if (this.logsContainer.current) {
-        this.logsContainer.current.scroll({
+      if (this.state.logsContainer) {
+        this.state.logsContainer.scroll({
           behavior: 'auto',
           top: 0,
         });
@@ -688,7 +693,7 @@ class UnthemedLogs extends PureComponent<Props, State> {
               </div>
             )}
             {this.state.visualisationType === 'logs' && hasData && (
-              <div className={styles.logRows} data-testid="logRows" ref={this.logsContainer}>
+              <div className={styles.logRows} data-testid="logRows" ref={this.onLogsContainerRef}>
                 <LogRows
                   logRows={logRows}
                   deduplicatedRows={dedupedRows}
@@ -715,6 +720,7 @@ class UnthemedLogs extends PureComponent<Props, State> {
                   permalinkedRowId={this.props.panelState?.logs?.id}
                   scrollIntoView={this.scrollIntoView}
                   isFilterLabelActive={this.props.isFilterLabelActive}
+                  containerRendered={!!this.state.logsContainer}
                 />
               </div>
             )}

public/app/features/logs/components/LogRow.test.tsx

@@ -61,7 +61,7 @@ describe('LogRow', () => {
   describe('with permalinking', () => {
     it('reports via feature tracking when log line matches', () => {
       const scrollIntoView = jest.fn();
-      setup({ permalinkedRowId: 'log-row-id', scrollIntoView });
+      setup({ permalinkedRowId: 'log-row-id', scrollIntoView, containerRendered: true });
       expect(reportInteraction).toHaveBeenCalledWith('grafana_explore_logs_permalink_opened', {
         logRowUid: 'log-row-id',
         datasourceType: 'unknown',
@@ -70,7 +70,11 @@ describe('LogRow', () => {
     });
 
     it('highlights row with same permalink-id', () => {
-      const { container } = setup({ permalinkedRowId: 'log-row-id' });
+      const { container } = setup({
+        permalinkedRowId: 'log-row-id',
+        containerRendered: true,
+        scrollIntoView: jest.fn(),
+      });
       const row = container.querySelector('tr');
       expect(row).toHaveStyle(
         `background-color: ${tinycolor(theme.colors.info.transparent).setAlpha(0.25).toString()}`
@@ -78,7 +82,12 @@ describe('LogRow', () => {
     });
 
     it('does not highlight row details with different permalink-id', async () => {
-      const { container } = setup({ permalinkedRowId: 'log-row-id', enableLogDetails: true });
+      const { container } = setup({
+        permalinkedRowId: 'log-row-id',
+        enableLogDetails: true,
+        containerRendered: true,
+        scrollIntoView: jest.fn(),
+      });
       const row = container.querySelector('tr');
       await userEvent.click(row!);
       const allRows = container.querySelectorAll('tr');
@@ -101,22 +110,28 @@ describe('LogRow', () => {
 
     it('calls `scrollIntoView` if permalink matches', () => {
       const scrollIntoView = jest.fn();
-      setup({ permalinkedRowId: 'log-row-id', scrollIntoView });
+      setup({ permalinkedRowId: 'log-row-id', scrollIntoView, containerRendered: true });
       expect(scrollIntoView).toHaveBeenCalled();
     });
 
     it('does not call `scrollIntoView` if permalink does not match', () => {
       const scrollIntoView = jest.fn();
-      setup({ permalinkedRowId: 'wrong-log-row-id', scrollIntoView });
+      setup({ permalinkedRowId: 'wrong-log-row-id', scrollIntoView, containerRendered: true });
       expect(scrollIntoView).not.toHaveBeenCalled();
     });
 
     it('calls `scrollIntoView` once', async () => {
       const scrollIntoView = jest.fn();
-      setup({ permalinkedRowId: 'log-row-id', scrollIntoView });
+      setup({ permalinkedRowId: 'log-row-id', scrollIntoView, containerRendered: true });
       await userEvent.hover(screen.getByText('test123'));
       expect(scrollIntoView).toHaveBeenCalledTimes(1);
     });
+
+    it('does not call `scrollIntoView` if permalink matches but container is not rendered yet', () => {
+      const scrollIntoView = jest.fn();
+      setup({ permalinkedRowId: 'log-row-id', scrollIntoView, containerRendered: false });
+      expect(scrollIntoView).not.toHaveBeenCalled();
+    });
   });
 
   it('should render the menu cell on mouse over', async () => {

public/app/features/logs/components/LogRow.tsx

@@ -46,6 +46,7 @@ interface Props extends Themeable2 {
   onPinLine?: (row: LogRowModel) => void;
   onUnpinLine?: (row: LogRowModel) => void;
   pinned?: boolean;
+  containerRendered?: boolean;
 }
 
 interface State {
@@ -135,7 +136,7 @@ class UnThemedLogRow extends PureComponent<Props, State> {
   }
 
   scrollToLogRow = (prevState: State, mounted = false) => {
-    const { row, permalinkedRowId, scrollIntoView } = this.props;
+    const { row, permalinkedRowId, scrollIntoView, containerRendered } = this.props;
 
     if (permalinkedRowId !== row.uid) {
       // only set the new state if the row is not permalinked anymore or if the component was mounted.
@@ -145,11 +146,9 @@ class UnThemedLogRow extends PureComponent<Props, State> {
       return;
     }
 
-    if (!this.state.permalinked) {
+    if (!this.state.permalinked && containerRendered && this.logLineRef.current && scrollIntoView) {
       // at this point this row is the permalinked row, so we need to scroll to it and highlight it if possible.
-      if (this.logLineRef.current && scrollIntoView) {
-        scrollIntoView(this.logLineRef.current);
-      }
+      scrollIntoView(this.logLineRef.current);
       reportInteraction('grafana_explore_logs_permalink_opened', {
         datasourceType: row.datasourceType ?? 'unknown',
         logRowUid: row.uid,

public/app/features/logs/components/LogRows.tsx

@@ -52,6 +52,7 @@ export interface Props extends Themeable2 {
   scrollIntoView?: (element: HTMLElement) => void;
   isFilterLabelActive?: (key: string, value: string) => Promise<boolean>;
   pinnedRowId?: string;
+  containerRendered?: boolean;
 }
 
 interface State {