Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 62 additions & 6 deletions static/app/views/explore/logs/pinning/PinnedLogs.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ import {

import {PinnedLogs} from 'sentry/views/explore/logs/pinning/PinnedLogs';
import {useLogsPinning} from 'sentry/views/explore/logs/pinning/useLogsPinning';
import {OurLogKnownFieldKey} from 'sentry/views/explore/logs/types';
import {
OurLogKnownFieldKey,
type OurLogsResponseItem,
} from 'sentry/views/explore/logs/types';
import type {LogTableRowItem} from 'sentry/views/explore/logs/utils';

const allRows: LogTableRowItem[] = [
Expand All @@ -33,18 +36,38 @@ const renderRow = (dataRow: LogTableRowItem) => (
</tr>
);

function PinnedLogsWrapper() {
function PinnedLogsWrapper({
fetchedPinnedRows = [],
isFetchingPinnedRows = false,
}: {
fetchedPinnedRows?: OurLogsResponseItem[];
isFetchingPinnedRows?: boolean;
}) {
const logsPinning = useLogsPinning()!;

return (
<table>
<PinnedLogs allRows={allRows} logsPinning={logsPinning} renderRow={renderRow} />
<PinnedLogs
allRows={allRows}
logsPinning={logsPinning}
query={{
fetchedRows: fetchedPinnedRows,
isPending: isFetchingPinnedRows,
}}
renderRow={renderRow}
/>
</table>
);
}

function renderPinnedLogs(options: RenderOptions = {}) {
return render(<PinnedLogsWrapper />, {
function renderPinnedLogs(
options: RenderOptions = {},
wrapperProps?: {
fetchedPinnedRows?: OurLogsResponseItem[];
isFetchingPinnedRows?: boolean;
}
) {
return render(<PinnedLogsWrapper {...wrapperProps} />, {
organization: {features: ['ourlogs-pinning']},
...options,
});
Expand All @@ -67,7 +90,40 @@ describe('PinnedLogs', () => {
expect(screen.getByTestId('pinned-row-log-1')).toBeInTheDocument();
});

it('does not render a row when the pinned id is missing from allRows', () => {
it('renders the pinned row when its id is present in fetchedPinnedRows but not allRows', () => {
const fetchedRow = LogFixture({
[OurLogKnownFieldKey.ID]: 'log-3',
[OurLogKnownFieldKey.PROJECT_ID]: '1',
[OurLogKnownFieldKey.ORGANIZATION_ID]: 1,
[OurLogKnownFieldKey.MESSAGE]: 'fetched pinned log',
});
renderPinnedLogs(
{
initialRouterConfig: {
location: {pathname: '/', query: {logsPinned: 'log-3'}},
},
},
{fetchedPinnedRows: [fetchedRow]}
);

expect(screen.getByTestId('pinned-row-log-3')).toBeInTheDocument();
});

it('shows a loading indicator for a pinned row that is not yet available', () => {
renderPinnedLogs(
{
initialRouterConfig: {
location: {pathname: '/', query: {logsPinned: 'missing-log'}},
},
},
{isFetchingPinnedRows: true}
);

expect(screen.getByTestId('loading-indicator')).toBeInTheDocument();
expect(screen.queryByTestId('pinned-row-missing-log')).not.toBeInTheDocument();
});

it('does not render a row when the pinned id is missing from allRows and not fetching', () => {
renderPinnedLogs({
initialRouterConfig: {
location: {pathname: '/', query: {logsPinned: 'missing-log'}},
Expand Down
32 changes: 27 additions & 5 deletions static/app/views/explore/logs/pinning/PinnedLogs.tsx
Original file line number Diff line number Diff line change
@@ -1,31 +1,46 @@
import {Fragment, useCallback, useState} from 'react';
import {Fragment, useCallback, useMemo, useState} from 'react';
import styled from '@emotion/styled';

import {Button} from '@sentry/scraps/button';
import {Flex} from '@sentry/scraps/layout';

import {LoadingIndicator} from 'sentry/components/loadingIndicator';
import {GridRow} from 'sentry/components/tables/gridEditable/styles';
import {IconChevron, IconClose} from 'sentry/icons';
import {t} from 'sentry/locale';
import {TableBody} from 'sentry/views/explore/components/table';
import type {LogsPinning} from 'sentry/views/explore/logs/pinning/useLogsPinning';
import type {usePinnedLogsQuery} from 'sentry/views/explore/logs/pinning/usePinnedLogsQuery';
import {OurLogKnownFieldKey} from 'sentry/views/explore/logs/types';
import type {LogTableRowItem} from 'sentry/views/explore/logs/utils';

interface Props {
allRows: LogTableRowItem[];
logsPinning: LogsPinning;
query: ReturnType<typeof usePinnedLogsQuery>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think query here is too generic of a name for what this parameter holds. I thought it was a string that represented a query to get the pinned logs. I think a better name might just be pinnedLogs

Alternatively, I was also wondering if we should just move the hook and this content into this component. It doesn't seem to be used by parent except to pass it down here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re the naming: yeah makes sense! I'll stew on this.

Re moving down: I tried that locally, but then it doesn't fire until the component renders - which adds a good chunk of delay.

renderRow: (dataRow: LogTableRowItem) => React.ReactNode;
}

export function PinnedLogs({allRows, logsPinning, renderRow}: Props) {
export function PinnedLogs({allRows, logsPinning, query, renderRow}: Props) {
const {fetchedRows: fetchedPinnedRows, isPending: isFetchingPinnedRows} = query;
const [expanded, setExpanded] = useState(true);
const pinnedRows = logsPinning.getPinnedRowIds();

const onInitialize = useCallback(() => {
setExpanded(true);
}, []);

const rowById = useMemo(() => {
const map = new Map<string, LogTableRowItem>();
for (const row of fetchedPinnedRows) {
map.set(row[OurLogKnownFieldKey.ID], row);
}
for (const row of allRows) {
map.set(row[OurLogKnownFieldKey.ID], row);
}
return map;
}, [allRows, fetchedPinnedRows]);

if (!pinnedRows.length) {
return null;
}
Expand All @@ -34,11 +49,18 @@ export function PinnedLogs({allRows, logsPinning, renderRow}: Props) {
<PinnedTableBody data-test-id="pinned-logs-table-body" ref={onInitialize}>
{expanded &&
pinnedRows.map(rowId => {
const dataRow = allRows.find(datum => datum[OurLogKnownFieldKey.ID] === rowId);
const dataRow = rowById.get(rowId);

// TODO(LOGS-781): this is not correct yet because the virtualizer might not have found it yet.
// Will have to manually re-fetch data.
if (!dataRow) {
if (isFetchingPinnedRows) {
return (
<GridRow key={rowId}>
<PinnedGridBodyCell>
<LoadingIndicator mini />
</PinnedGridBodyCell>
</GridRow>
);
}
return null;
}

Expand Down
28 changes: 28 additions & 0 deletions static/app/views/explore/logs/pinning/useLogsPinning.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,33 @@ describe('useLogsPinning', () => {

expect(result.current?.getPinnedRowIds()).toEqual([]);
});

it('removes the id from pinnedRows when removePinnedRows is called for a pinned id', () => {
const {result} = renderHookWithProviders(() => useLogsPinning(), {
initialRouterConfig: {
location: {pathname: '/', query: {logsPinned: 'log-1'}},
},
});

act(() => {
result.current?.removePinnedRows(['log-1']);
});

expect(result.current?.getPinnedRowIds()).toEqual([]);
});

it('removes all targeted ids when removePinnedRows is called with multiple ids', () => {
const {result} = renderHookWithProviders(() => useLogsPinning());

act(() => result.current?.togglePinnedRow('log-1'));
act(() => result.current?.togglePinnedRow('log-2'));
act(() => result.current?.togglePinnedRow('log-3'));

act(() => {
result.current?.removePinnedRows(['log-1', 'log-2']);
});

expect(result.current?.getPinnedRowIds()).toEqual(['log-3']);
});
});
});
21 changes: 19 additions & 2 deletions static/app/views/explore/logs/pinning/useLogsPinning.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export interface LogsPinning {
clearPinnedRows: () => void;
getPinnedRowIds: () => string[];
hasPinnedRow: (id: string) => boolean;
removePinnedRows: (ids: string[]) => void;
togglePinnedRow: (id: string) => void;
}

Expand Down Expand Up @@ -39,13 +40,29 @@ export function useLogsPinning(): LogsPinning | undefined {
[setPinnedRowsList]
);

const removePinnedRows = useCallback(
(ids: string[]) => {
const idsToRemove = new Set(ids);
setPinnedRowsList(previous =>
previous.filter(previousId => !idsToRemove.has(previousId))
);
},
[setPinnedRowsList]
);

const clearPinnedRows = useCallback(() => {
setPinnedRowsList([]);
}, [setPinnedRowsList]);

const value = useMemo(
() => ({clearPinnedRows, getPinnedRowIds, hasPinnedRow, togglePinnedRow}),
[clearPinnedRows, getPinnedRowIds, hasPinnedRow, togglePinnedRow]
() => ({
clearPinnedRows,
getPinnedRowIds,
hasPinnedRow,
removePinnedRows,
togglePinnedRow,
}),
[clearPinnedRows, getPinnedRowIds, hasPinnedRow, removePinnedRows, togglePinnedRow]
);

return logsPinningEnabled ? value : undefined;
Expand Down
Loading
Loading