From a676fd620a861df3f53fe73f000390812208de05 Mon Sep 17 00:00:00 2001 From: Matias Chomicki Date: Thu, 22 May 2025 20:12:06 +0200 Subject: [PATCH] New Logs Panel: follow up improvements and fixes (#105767) * LogList: add version with no scroll * LogList: fix support for permalinked and pinned logs * LogLineDetails: improve resizable * LogList: improve height adjustment when details are open * LogList: let the people select text * Revert "LogList: add version with no scroll" This reverts commit f26cdce696cd297d0fc1a295330a631ba29bc9b9. * LogList: update test * New Logs Panel: rename permalinkedRowId to permalinkedLogId * LogListContext: update mock * ControlledLogRows: implement custom scrollIntoView * Logs Panel: fix re-render regression * LogLine: tweak hover and expanded colors --- public/app/features/explore/Logs/Logs.tsx | 2 +- .../logs/components/ControlledLogRows.tsx | 31 +++++++++++++++++-- .../logs/components/panel/LogLine.tsx | 12 ++++--- .../logs/components/panel/LogLineDetails.tsx | 11 +++++-- .../logs/components/panel/LogList.test.tsx | 20 ++++++++++++ .../logs/components/panel/LogList.tsx | 30 +++++++++++++++--- .../components/panel/LogListContext.test.tsx | 26 ++++++++++++++-- .../logs/components/panel/LogListContext.tsx | 19 +++++++++--- .../panel/__mocks__/LogListContext.tsx | 12 +++++-- 9 files changed, 141 insertions(+), 22 deletions(-) diff --git a/public/app/features/explore/Logs/Logs.tsx b/public/app/features/explore/Logs/Logs.tsx index fb61d584ee6..6025d4f8c9e 100644 --- a/public/app/features/explore/Logs/Logs.tsx +++ b/public/app/features/explore/Logs/Logs.tsx @@ -1063,7 +1063,6 @@ const UnthemedLogs: React.FunctionComponent = (props: Props) => { onOpenContext={onOpenContext} onPermalinkClick={onPermalinkClick} permalinkedRowId={panelState?.logs?.id} - scrollIntoView={scrollIntoView} isFilterLabelActive={props.isFilterLabelActive} onClickFilterString={props.onClickFilterString} onClickFilterOutString={props.onClickFilterOutString} @@ -1179,6 +1178,7 @@ const UnthemedLogs: React.FunctionComponent = (props: Props) => { onPermalinkClick={onPermalinkClick} onPinLine={onPinToContentOutlineClick} onUnpinLine={onPinToContentOutlineClick} + permalinkedLogId={panelState?.logs?.id} pinLineButtonTooltipTitle={pinLineButtonTooltipTitle} pinnedLogs={pinnedLogs} showControls diff --git a/public/app/features/logs/components/ControlledLogRows.tsx b/public/app/features/logs/components/ControlledLogRows.tsx index f48558c1c86..240e96e6b17 100644 --- a/public/app/features/logs/components/ControlledLogRows.tsx +++ b/public/app/features/logs/components/ControlledLogRows.tsx @@ -1,5 +1,5 @@ import { css } from '@emotion/css'; -import { useEffect, useMemo, useRef, forwardRef, useImperativeHandle } from 'react'; +import { useEffect, useMemo, useRef, forwardRef, useImperativeHandle, useCallback } from 'react'; import { AbsoluteTimeRange, @@ -96,7 +96,17 @@ export const ControlledLogRows = forwardRef( - ({ loading, loadMoreLogs, deduplicatedRows = [], range, ...rest }: LogRowsComponentProps, ref) => { + ( + { + loading, + loadMoreLogs, + deduplicatedRows = [], + range, + scrollIntoView: scrollIntoViewProp, + ...rest + }: LogRowsComponentProps, + ref + ) => { const { app, dedupStrategy, @@ -135,6 +145,22 @@ const LogRowsComponent = forwardRef { + if (scrollIntoViewProp) { + scrollIntoViewProp(element); + return; + } + if (scrollElementRef.current) { + scrollElementRef.current.scroll({ + behavior: 'smooth', + top: scrollElementRef.current.scrollTop + element.getBoundingClientRect().top - window.innerHeight / 2, + }); + } + }, + [scrollIntoViewProp] + ); + return (
@@ -158,6 +184,7 @@ const LogRowsComponent = forwardRef(null); const pinned = useLogIsPinned(log); + const permalinked = useLogIsPermalinked(log); useEffect(() => { if (!onOverflow || !logLineRef.current) { @@ -92,7 +93,7 @@ export const LogLine = ({ return (
{ fontSize: theme.typography.fontSize, wordBreak: 'break-all', '&:hover': { - background: `hsla(0, 0%, 0%, 0.2)`, + background: theme.isDark ? `hsla(0, 0%, 0%, 0.3)` : `hsla(0, 0%, 0%, 0.1)`, }, '&.infinite-scroll': { '&::before': { @@ -311,11 +312,14 @@ export const getStyles = (theme: GrafanaTheme2) => { }, }), detailsDisplayed: css({ - background: `hsla(0, 0%, 0%, 0.2)`, + background: theme.isDark ? `hsla(0, 0%, 0%, 0.5)` : `hsla(0, 0%, 0%, 0.1)`, }), pinnedLogLine: css({ backgroundColor: tinycolor(theme.colors.info.transparent).setAlpha(0.25).toString(), }), + permalinkedLogLine: css({ + backgroundColor: tinycolor(theme.colors.info.transparent).setAlpha(0.25).toString(), + }), menuIcon: css({ height: getLineHeight(), margin: 0, diff --git a/public/app/features/logs/components/panel/LogLineDetails.tsx b/public/app/features/logs/components/panel/LogLineDetails.tsx index d290e43be29..df55103225a 100644 --- a/public/app/features/logs/components/panel/LogLineDetails.tsx +++ b/public/app/features/logs/components/panel/LogLineDetails.tsx @@ -4,7 +4,7 @@ import { useCallback, useRef } from 'react'; import { GrafanaTheme2 } from '@grafana/data'; import { useTranslate } from '@grafana/i18n'; -import { IconButton, useStyles2, useTheme2 } from '@grafana/ui'; +import { getDragStyles, IconButton, useStyles2, useTheme2 } from '@grafana/ui'; import { GetFieldLinksFn } from 'app/plugins/panel/logs/types'; import { LogDetails } from '../LogDetails'; @@ -40,6 +40,7 @@ export const LogLineDetails = ({ containerElement, getFieldLinks, logs, onResize const getRows = useCallback(() => logs, [logs]); const logRowsStyles = getLogRowStyles(useTheme2()); const styles = useStyles2(getStyles); + const dragStyles = useStyles2(getDragStyles); const containerRef = useRef(null); const { t } = useTranslate(); @@ -51,7 +52,13 @@ export const LogLineDetails = ({ containerElement, getFieldLinks, logs, onResize }, [onResize, setDetailsWidth]); return ( - +
{ expect(screen.queryByText('Fields')).not.toBeInTheDocument(); expect(screen.queryByText('Close log details')).not.toBeInTheDocument(); }); + + test('Allows people to select text without opening log details', async () => { + const spy = jest.spyOn(document, 'getSelection'); + spy.mockReturnValue({ + toString: () => 'selected log line', + removeAllRanges: () => {}, + addRange: (range: Range) => {}, + } as Selection); + + render(); + + await userEvent.click(screen.getByText('log message 1')); + + expect(screen.queryByText('name_of_the_label')).not.toBeInTheDocument(); + expect(screen.queryByText('value of the label')).not.toBeInTheDocument(); + expect(screen.queryByText('Fields')).not.toBeInTheDocument(); + expect(screen.queryByText('Close log details')).not.toBeInTheDocument(); + + spy.mockRestore(); + }); }); diff --git a/public/app/features/logs/components/panel/LogList.tsx b/public/app/features/logs/components/panel/LogList.tsx index f55a670b526..1dc80967a90 100644 --- a/public/app/features/logs/components/panel/LogList.tsx +++ b/public/app/features/logs/components/panel/LogList.tsx @@ -69,6 +69,7 @@ export interface Props { onPinLine?: (row: LogRowModel) => void; onOpenContext?: (row: LogRowModel, onClose: () => void) => void; onUnpinLine?: (row: LogRowModel) => void; + permalinkedLogId?: string; pinLineButtonTooltipTitle?: PopoverContent; pinnedLogs?: string[]; showControls: boolean; @@ -88,6 +89,7 @@ type LogListComponentProps = Omit< | 'dedupStrategy' | 'displayedFields' | 'enableLogDetails' + | 'permalinkedLogId' | 'showTime' | 'sortOrder' | 'syntaxHighlighting' @@ -125,6 +127,7 @@ export const LogList = ({ onPinLine, onOpenContext, onUnpinLine, + permalinkedLogId, pinLineButtonTooltipTitle, pinnedLogs, showControls, @@ -161,6 +164,7 @@ export const LogList = ({ onPinLine={onPinLine} onOpenContext={onOpenContext} onUnpinLine={onUnpinLine} + permalinkedLogId={permalinkedLogId} pinLineButtonTooltipTitle={pinLineButtonTooltipTitle} pinnedLogs={pinnedLogs} showControls={showControls} @@ -205,6 +209,7 @@ const LogListComponent = ({ dedupStrategy, filterLevels, forceEscape, + permalinkedLogId, showDetails, showTime, sortOrder, @@ -213,7 +218,9 @@ const LogListComponent = ({ } = useLogListContext(); const [processedLogs, setProcessedLogs] = useState([]); const [listHeight, setListHeight] = useState( - app === CoreApp.Explore ? window.innerHeight * 0.75 : containerElement.clientHeight + app === CoreApp.Explore + ? Math.max(window.innerHeight * 0.8, containerElement.clientHeight) + : containerElement.clientHeight ); const theme = useTheme2(); const listRef = useRef(null); @@ -262,7 +269,11 @@ const LogListComponent = ({ useEffect(() => { const handleResize = debounce(() => { - setListHeight(app === CoreApp.Explore ? window.innerHeight * 0.75 : containerElement.clientHeight); + setListHeight( + app === CoreApp.Explore + ? Math.max(window.innerHeight * 0.8, containerElement.clientHeight) + : containerElement.clientHeight + ); }, 50); window.addEventListener('resize', handleResize); handleResize(); @@ -292,8 +303,15 @@ const LogListComponent = ({ ); const handleScrollPosition = useCallback(() => { - listRef.current?.scrollToItem(initialScrollPosition === 'top' ? 0 : logs.length - 1); - }, [initialScrollPosition, logs.length]); + if (permalinkedLogId) { + const index = processedLogs.findIndex((log) => log.uid === permalinkedLogId); + if (index >= 0) { + listRef.current?.scrollToItem(index, 'start'); + return; + } + } + listRef.current?.scrollToItem(initialScrollPosition === 'top' ? 0 : processedLogs.length - 1); + }, [initialScrollPosition, permalinkedLogId, processedLogs]); if (!containerElement || listHeight == null) { // Wait for container to be rendered @@ -302,6 +320,10 @@ const LogListComponent = ({ const handleLogLineClick = useCallback( (log: LogListModel) => { + // Let people select text + if (document.getSelection()?.toString()) { + return; + } toggleDetails(log); }, [toggleDetails] diff --git a/public/app/features/logs/components/panel/LogListContext.test.tsx b/public/app/features/logs/components/panel/LogListContext.test.tsx index 8010efcdf74..efe2281d8b9 100644 --- a/public/app/features/logs/components/panel/LogListContext.test.tsx +++ b/public/app/features/logs/components/panel/LogListContext.test.tsx @@ -3,13 +3,20 @@ import { ReactNode } from 'react'; import { createLogLine } from '../__mocks__/logRow'; -import { useLogListContextData, useLogListContext, useLogIsPinned, LogListContext } from './LogListContext'; +import { + useLogListContextData, + useLogListContext, + useLogIsPinned, + LogListContext, + useLogIsPermalinked, +} from './LogListContext'; import { defaultValue } from './__mocks__/LogListContext'; -const log = createLogLine({ rowId: 'yep' }); +const log = createLogLine({ rowId: 'yep', uid: 'uid' }); const value = { ...defaultValue, pinnedLogs: ['yep'], + permalinkedLogId: log.uid, }; const wrapper = ({ children }: { children: ReactNode }) => ( {children} @@ -33,9 +40,22 @@ test('Allows to tell if a log is pinned', () => { expect(result.current).toBe(true); }); -test('Allows to tell if a log is pinned', () => { +test('Allows to tell if a log is not pinned', () => { const otherLog = createLogLine({ rowId: 'nope' }); const { result } = renderHook(() => useLogIsPinned(otherLog), { wrapper }); expect(result.current).toBe(false); }); + +test('Allows to tell if a log is permalinked', () => { + const { result } = renderHook(() => useLogIsPermalinked(log), { wrapper }); + + expect(result.current).toBe(true); +}); + +test('Allows to tell if a log is not permalinked', () => { + const otherLog = createLogLine({ rowId: 'nope' }); + const { result } = renderHook(() => useLogIsPermalinked(otherLog), { wrapper }); + + expect(result.current).toBe(false); +}); diff --git a/public/app/features/logs/components/panel/LogListContext.tsx b/public/app/features/logs/components/panel/LogListContext.tsx index 2e9b35ee165..546643fc813 100644 --- a/public/app/features/logs/components/panel/LogListContext.tsx +++ b/public/app/features/logs/components/panel/LogListContext.tsx @@ -91,11 +91,16 @@ export const useLogListContext = (): LogListContextData => { return useContext(LogListContext); }; -export const useLogIsPinned = (log: LogRowModel) => { +export const useLogIsPinned = (log: LogListModel) => { const { pinnedLogs } = useContext(LogListContext); return pinnedLogs?.some((logId) => logId === log.rowId); }; +export const useLogIsPermalinked = (log: LogListModel) => { + const { permalinkedLogId } = useContext(LogListContext); + return permalinkedLogId && permalinkedLogId === log.uid; +}; + export type LogListState = Pick< LogListContextData, | 'dedupStrategy' @@ -139,6 +144,7 @@ export interface Props { onPinLine?: (row: LogRowModel) => void; onOpenContext?: (row: LogRowModel, onClose: () => void) => void; onUnpinLine?: (row: LogRowModel) => void; + permalinkedLogId?: string; pinLineButtonTooltipTitle?: PopoverContent; pinnedLogs?: string[]; prettifyJSON?: boolean; @@ -178,6 +184,7 @@ export const LogListContextProvider = ({ onPinLine, onOpenContext, onUnpinLine, + permalinkedLogId, pinLineButtonTooltipTitle, pinnedLogs, prettifyJSON, @@ -217,9 +224,6 @@ export const LogListContextProvider = ({ syntaxHighlighting, wrapLogMessage, }; - if (!shallowCompare(logListState.pinnedLogs ?? [], pinnedLogs ?? [])) { - newState.pinnedLogs = pinnedLogs; - } if (!shallowCompare(logListState, newState)) { setLogListState(newState); } @@ -250,6 +254,12 @@ export const LogListContextProvider = ({ } }, [hasUnescapedContent, logListState]); + useEffect(() => { + if (!shallowCompare(logListState.pinnedLogs ?? [], pinnedLogs ?? [])) { + setLogListState({ ...logListState, pinnedLogs }); + } + }, [logListState, pinnedLogs]); + const detailsDisplayed = useCallback( (log: LogListModel) => !!showDetails.find((shownLog) => shownLog.uid === log.uid), [showDetails] @@ -426,6 +436,7 @@ export const LogListContextProvider = ({ onPinLine, onOpenContext, onUnpinLine, + permalinkedLogId, pinLineButtonTooltipTitle, pinnedLogs: logListState.pinnedLogs, prettifyJSON: logListState.prettifyJSON, diff --git a/public/app/features/logs/components/panel/__mocks__/LogListContext.tsx b/public/app/features/logs/components/panel/__mocks__/LogListContext.tsx index 53f002a2a2d..e9fcaabdeca 100644 --- a/public/app/features/logs/components/panel/__mocks__/LogListContext.tsx +++ b/public/app/features/logs/components/panel/__mocks__/LogListContext.tsx @@ -1,8 +1,9 @@ import { createContext, useContext } from 'react'; -import { CoreApp, LogRowModel, LogsDedupStrategy, LogsSortOrder } from '@grafana/data'; +import { CoreApp, LogsDedupStrategy, LogsSortOrder } from '@grafana/data'; import { LogListContextData, Props } from '../LogListContext'; +import { LogListModel } from '../processing'; export const LogListContext = createContext({ app: CoreApp.Unknown, @@ -44,11 +45,16 @@ export const useLogListContext = (): LogListContextData => { return useContext(LogListContext); }; -export const useLogIsPinned = (log: LogRowModel) => { +export const useLogIsPinned = (log: LogListModel) => { const { pinnedLogs } = useContext(LogListContext); return pinnedLogs?.some((logId) => logId === log.rowId); }; +export const useLogIsPermalinked = (log: LogListModel) => { + const { permalinkedLogId } = useContext(LogListContext); + return permalinkedLogId && permalinkedLogId === log.uid; +}; + export const defaultValue: LogListContextData = { setDedupStrategy: jest.fn(), setFilterLevels: jest.fn(), @@ -114,6 +120,7 @@ export const LogListContextProvider = ({ onPinLine = jest.fn(), onOpenContext = jest.fn(), onUnpinLine = jest.fn(), + permalinkedLogId, pinnedLogs = [], showTime = true, sortOrder = LogsSortOrder.Descending, @@ -137,6 +144,7 @@ export const LogListContextProvider = ({ onPinLine, onOpenContext, onUnpinLine, + permalinkedLogId, pinnedLogs, setDedupStrategy: jest.fn(), setFilterLevels: jest.fn(),