From 394b9bc0b4ac77a2dc310bef005299ca1b74bde4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Mon, 1 Jun 2026 15:22:17 -0400 Subject: [PATCH 1/2] feat(ourlogs): Fetch not-yet-available pinned logs and invalidate on filter changes Adds data fetching for pinned log rows that are in the URL but not yet loaded in the infinite scroll table (e.g. the virtualizer hasn't reached them yet). Also removes pinned IDs from the URL when they can't be found within the current page filters (date range, project, environment). - Add removePinnedRow to LogsPinning interface (idempotent removal) - New usePinnedLogsQuery hook: fetches missing pinned log rows via the events API, then removes any IDs that aren't returned (invalidation) - PinnedLogs now accepts fetchedPinnedRows/isFetchingPinnedRows props, merges fetched rows with existing allRows, and shows a loading indicator while rows are being fetched - logsInfiniteTable calls usePinnedLogsQuery and passes results down Refs LOGS-781 Co-Authored-By: Claude Sonnet 4 --- .../explore/logs/pinning/PinnedLogs.spec.tsx | 63 ++++- .../views/explore/logs/pinning/PinnedLogs.tsx | 32 ++- .../logs/pinning/useLogsPinning.spec.tsx | 14 + .../explore/logs/pinning/useLogsPinning.tsx | 18 +- .../logs/pinning/usePinnedLogsQuery.spec.tsx | 243 ++++++++++++++++++ .../logs/pinning/usePinnedLogsQuery.tsx | 95 +++++++ .../explore/logs/tables/logsInfiniteTable.tsx | 9 +- 7 files changed, 461 insertions(+), 13 deletions(-) create mode 100644 static/app/views/explore/logs/pinning/usePinnedLogsQuery.spec.tsx create mode 100644 static/app/views/explore/logs/pinning/usePinnedLogsQuery.tsx diff --git a/static/app/views/explore/logs/pinning/PinnedLogs.spec.tsx b/static/app/views/explore/logs/pinning/PinnedLogs.spec.tsx index 06e0102400eda6..6d21eee67551f5 100644 --- a/static/app/views/explore/logs/pinning/PinnedLogs.spec.tsx +++ b/static/app/views/explore/logs/pinning/PinnedLogs.spec.tsx @@ -33,18 +33,38 @@ const renderRow = (dataRow: LogTableRowItem) => ( ); -function PinnedLogsWrapper() { +function PinnedLogsWrapper({ + fetchedPinnedRows = [], + isFetchingPinnedRows = false, +}: { + fetchedPinnedRows?: LogTableRowItem[]; + isFetchingPinnedRows?: boolean; +}) { const logsPinning = useLogsPinning()!; return ( - +
); } -function renderPinnedLogs(options: RenderOptions = {}) { - return render(, { +function renderPinnedLogs( + options: RenderOptions = {}, + wrapperProps?: { + fetchedPinnedRows?: LogTableRowItem[]; + isFetchingPinnedRows?: boolean; + } +) { + return render(, { organization: {features: ['ourlogs-pinning']}, ...options, }); @@ -67,7 +87,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'}}, diff --git a/static/app/views/explore/logs/pinning/PinnedLogs.tsx b/static/app/views/explore/logs/pinning/PinnedLogs.tsx index a1d23e248ccddd..2aeea469d30dd1 100644 --- a/static/app/views/explore/logs/pinning/PinnedLogs.tsx +++ b/static/app/views/explore/logs/pinning/PinnedLogs.tsx @@ -1,24 +1,28 @@ -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; 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(); @@ -26,6 +30,17 @@ export function PinnedLogs({allRows, logsPinning, renderRow}: Props) { setExpanded(true); }, []); + const rowById = useMemo(() => { + const map = new Map(); + 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; } @@ -34,11 +49,18 @@ export function PinnedLogs({allRows, logsPinning, renderRow}: Props) { {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 ( + + + + + + ); + } return null; } diff --git a/static/app/views/explore/logs/pinning/useLogsPinning.spec.tsx b/static/app/views/explore/logs/pinning/useLogsPinning.spec.tsx index 0887891a1ef94f..9220c8f96ae8d3 100644 --- a/static/app/views/explore/logs/pinning/useLogsPinning.spec.tsx +++ b/static/app/views/explore/logs/pinning/useLogsPinning.spec.tsx @@ -101,5 +101,19 @@ describe('useLogsPinning', () => { expect(result.current?.getPinnedRowIds()).toEqual([]); }); + + it('removes the id from pinnedRows when removePinnedRow is called for a pinned id', () => { + const {result} = renderHookWithProviders(() => useLogsPinning(), { + initialRouterConfig: { + location: {pathname: '/', query: {logsPinned: 'log-1'}}, + }, + }); + + act(() => { + result.current?.removePinnedRow('log-1'); + }); + + expect(result.current?.getPinnedRowIds()).toEqual([]); + }); }); }); diff --git a/static/app/views/explore/logs/pinning/useLogsPinning.tsx b/static/app/views/explore/logs/pinning/useLogsPinning.tsx index 5aaf752b3ed9ee..ba61bc5089f625 100644 --- a/static/app/views/explore/logs/pinning/useLogsPinning.tsx +++ b/static/app/views/explore/logs/pinning/useLogsPinning.tsx @@ -9,6 +9,7 @@ export interface LogsPinning { clearPinnedRows: () => void; getPinnedRowIds: () => string[]; hasPinnedRow: (id: string) => boolean; + removePinnedRow: (id: string) => void; togglePinnedRow: (id: string) => void; } @@ -39,13 +40,26 @@ export function useLogsPinning(): LogsPinning | undefined { [setPinnedRowsList] ); + const removePinnedRow = useCallback( + (id: string) => { + setPinnedRowsList(pinnedRowsList.filter(previousId => previousId !== id)); + }, + [pinnedRowsList, setPinnedRowsList] + ); + const clearPinnedRows = useCallback(() => { setPinnedRowsList([]); }, [setPinnedRowsList]); const value = useMemo( - () => ({clearPinnedRows, getPinnedRowIds, hasPinnedRow, togglePinnedRow}), - [clearPinnedRows, getPinnedRowIds, hasPinnedRow, togglePinnedRow] + () => ({ + clearPinnedRows, + getPinnedRowIds, + hasPinnedRow, + removePinnedRow, + togglePinnedRow, + }), + [clearPinnedRows, getPinnedRowIds, hasPinnedRow, removePinnedRow, togglePinnedRow] ); return logsPinningEnabled ? value : undefined; diff --git a/static/app/views/explore/logs/pinning/usePinnedLogsQuery.spec.tsx b/static/app/views/explore/logs/pinning/usePinnedLogsQuery.spec.tsx new file mode 100644 index 00000000000000..8d13e133f7dbb7 --- /dev/null +++ b/static/app/views/explore/logs/pinning/usePinnedLogsQuery.spec.tsx @@ -0,0 +1,243 @@ +import {LogFixture} from 'sentry-fixture/log'; +import {OrganizationFixture} from 'sentry-fixture/organization'; +import {ProjectFixture} from 'sentry-fixture/project'; + +import {act, renderHookWithProviders, waitFor} from 'sentry-test/reactTestingLibrary'; + +import {PageFiltersStore} from 'sentry/components/pageFilters/store'; +import {ProjectsStore} from 'sentry/stores/projectsStore'; +import {LogsAnalyticsPageSource} from 'sentry/utils/analytics/logsAnalyticsEvent'; +import {LogsQueryParamsProvider} from 'sentry/views/explore/logs/logsQueryParamsProvider'; +import type {LogsPinning} from 'sentry/views/explore/logs/pinning/useLogsPinning'; +import {usePinnedLogsQuery} from 'sentry/views/explore/logs/pinning/usePinnedLogsQuery'; +import {OurLogKnownFieldKey} from 'sentry/views/explore/logs/types'; + +const organization = OrganizationFixture({ + features: ['ourlogs-enabled', 'ourlogs-pinning'], +}); +const project = ProjectFixture(); + +function makeLogsPinning(pinnedIds: string[]): LogsPinning { + return { + clearPinnedRows: jest.fn(), + getPinnedRowIds: jest.fn().mockReturnValue(pinnedIds), + hasPinnedRow: jest.fn((id: string) => pinnedIds.includes(id)), + removePinnedRow: jest.fn(), + togglePinnedRow: jest.fn(), + }; +} + +function AdditionalWrapper({children}: {children: React.ReactNode}) { + return ( + + {children} + + ); +} + +describe('usePinnedLogsQuery', () => { + beforeEach(() => { + MockApiClient.clearMockResponses(); + ProjectsStore.loadInitialData([project]); + PageFiltersStore.init(); + PageFiltersStore.onInitializeUrlState({ + projects: [parseInt(project.id, 10)], + environments: [], + datetime: { + period: '14d', + start: null, + end: null, + utc: null, + }, + }); + }); + + it('returns empty fetchedRows when all pinned ids are in allRows', () => { + const logRow = LogFixture({ + [OurLogKnownFieldKey.ID]: 'log-1', + [OurLogKnownFieldKey.PROJECT_ID]: String(project.id), + }); + const logsPinning = makeLogsPinning(['log-1']); + + const {result} = renderHookWithProviders( + () => usePinnedLogsQuery({allRows: [logRow], logsPinning}), + {organization, additionalWrapper: AdditionalWrapper} + ); + + expect(result.current.fetchedRows).toEqual([]); + expect(result.current.isPending).toBe(false); + }); + + it('returns empty fetchedRows when logsPinning is undefined', () => { + const {result} = renderHookWithProviders( + () => usePinnedLogsQuery({allRows: [], logsPinning: undefined}), + {organization, additionalWrapper: AdditionalWrapper} + ); + + expect(result.current.fetchedRows).toEqual([]); + expect(result.current.isPending).toBe(false); + }); + + it('fetches missing pinned rows from the API', async () => { + const missingLog = LogFixture({ + [OurLogKnownFieldKey.ID]: 'log-missing', + [OurLogKnownFieldKey.PROJECT_ID]: String(project.id), + [OurLogKnownFieldKey.MESSAGE]: 'fetched log', + }); + + const eventsRequest = MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/events/`, + method: 'GET', + body: { + data: [missingLog], + meta: {fields: {id: 'string'}, units: {}}, + }, + }); + + const logsPinning = makeLogsPinning(['log-missing']); + + const {result} = renderHookWithProviders( + () => usePinnedLogsQuery({allRows: [], logsPinning}), + {organization, additionalWrapper: AdditionalWrapper} + ); + + await waitFor(() => { + expect(result.current.fetchedRows).toHaveLength(1); + }); + + expect(eventsRequest).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + query: expect.objectContaining({ + query: 'id:[log-missing]', + dataset: 'ourlogs', + sampling: 'HIGHEST_ACCURACY', + }), + }) + ); + expect(result.current.fetchedRows[0]?.[OurLogKnownFieldKey.ID]).toBe('log-missing'); + }); + + it('does not call removePinnedRow when the scan was only partial', async () => { + MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/events/`, + method: 'GET', + body: { + data: [], + meta: {fields: {id: 'string'}, units: {}, dataScanned: 'partial'}, + }, + }); + + const logsPinning = makeLogsPinning(['log-not-scanned']); + + const {result} = renderHookWithProviders( + () => usePinnedLogsQuery({allRows: [], logsPinning}), + {organization, additionalWrapper: AdditionalWrapper} + ); + + await waitFor(() => { + expect(result.current.isPending).toBe(false); + }); + + expect(logsPinning.removePinnedRow).not.toHaveBeenCalled(); + }); + + it('calls removePinnedRow for ids not found in the API response', async () => { + MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/events/`, + method: 'GET', + body: { + data: [], + meta: {fields: {id: 'string'}, units: {}}, + }, + }); + + const logsPinning = makeLogsPinning(['log-gone']); + + renderHookWithProviders(() => usePinnedLogsQuery({allRows: [], logsPinning}), { + organization, + additionalWrapper: AdditionalWrapper, + }); + + await waitFor(() => { + expect(logsPinning.removePinnedRow).toHaveBeenCalledWith('log-gone'); + }); + }); + + it('does not call removePinnedRow for ids that are found in the API response', async () => { + const foundLog = LogFixture({ + [OurLogKnownFieldKey.ID]: 'log-found', + [OurLogKnownFieldKey.PROJECT_ID]: String(project.id), + }); + + MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/events/`, + method: 'GET', + body: { + data: [foundLog], + meta: {fields: {id: 'string'}, units: {}}, + }, + }); + + const logsPinning = makeLogsPinning(['log-found']); + + renderHookWithProviders(() => usePinnedLogsQuery({allRows: [], logsPinning}), { + organization, + additionalWrapper: AdditionalWrapper, + }); + + await waitFor(() => { + expect(logsPinning.removePinnedRow).not.toHaveBeenCalled(); + }); + }); + + it('is pending while fetching missing rows', async () => { + let resolveRequest!: (value: unknown) => void; + MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/events/`, + method: 'GET', + asyncDelay: new Promise(resolve => { + resolveRequest = resolve; + }) as any, + body: {data: [], meta: {fields: {}, units: {}}}, + }); + + const logsPinning = makeLogsPinning(['log-pending']); + + const {result} = renderHookWithProviders( + () => usePinnedLogsQuery({allRows: [], logsPinning}), + {organization, additionalWrapper: AdditionalWrapper} + ); + + expect(result.current.isPending).toBe(true); + + act(() => { + resolveRequest({}); + }); + }); + + it('does not fetch when pinned ids are already in allRows', () => { + const existingLog = LogFixture({ + [OurLogKnownFieldKey.ID]: 'log-existing', + [OurLogKnownFieldKey.PROJECT_ID]: String(project.id), + }); + + const eventsRequest = MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/events/`, + method: 'GET', + body: {data: [], meta: {fields: {}, units: {}}}, + }); + + const logsPinning = makeLogsPinning(['log-existing']); + + renderHookWithProviders( + () => usePinnedLogsQuery({allRows: [existingLog], logsPinning}), + {organization, additionalWrapper: AdditionalWrapper} + ); + + expect(eventsRequest).not.toHaveBeenCalled(); + }); +}); diff --git a/static/app/views/explore/logs/pinning/usePinnedLogsQuery.tsx b/static/app/views/explore/logs/pinning/usePinnedLogsQuery.tsx new file mode 100644 index 00000000000000..9cd5410f7e6b08 --- /dev/null +++ b/static/app/views/explore/logs/pinning/usePinnedLogsQuery.tsx @@ -0,0 +1,95 @@ +import {useEffect, useMemo} from 'react'; +import {skipToken, useQuery} from '@tanstack/react-query'; + +import {usePageFilters} from 'sentry/components/pageFilters/usePageFilters'; +import {apiOptions} from 'sentry/utils/api/apiOptions'; +import {DiscoverDatasets} from 'sentry/utils/discover/types'; +import {useOrganization} from 'sentry/utils/useOrganization'; +import {SAMPLING_MODE} from 'sentry/views/explore/hooks/useProgressiveQuery'; +import {AlwaysPresentLogFields} from 'sentry/views/explore/logs/constants'; +import type {LogsPinning} from 'sentry/views/explore/logs/pinning/useLogsPinning'; +import { + OurLogKnownFieldKey, + type EventsLogsResult, +} from 'sentry/views/explore/logs/types'; +import type {LogTableRowItem} from 'sentry/views/explore/logs/utils'; +import {useQueryParamsFields} from 'sentry/views/explore/queryParams/context'; + +interface PinnedLogsOptions { + allRows: LogTableRowItem[]; + logsPinning: LogsPinning | undefined; +} + +export function usePinnedLogsQuery({allRows, logsPinning}: PinnedLogsOptions) { + const organization = useOrganization(); + const {selection, isReady: pageFiltersReady} = usePageFilters(); + const userFields = useQueryParamsFields(); + + const allRowIds = useMemo( + () => new Set(allRows.map(row => row[OurLogKnownFieldKey.ID])), + [allRows] + ); + + const missingIds = useMemo(() => { + const pinnedIds = logsPinning?.getPinnedRowIds() ?? []; + return pinnedIds.filter(id => !allRowIds.has(id)); + }, [logsPinning, allRowIds]); + + const fields = useMemo( + () => Array.from(new Set([...AlwaysPresentLogFields, ...userFields])), + [userFields] + ); + + const shouldFetch = missingIds.length > 0 && pageFiltersReady && !!logsPinning; + + const queryResult = useQuery( + apiOptions.as()('/organizations/$organizationIdOrSlug/events/', { + path: shouldFetch ? {organizationIdOrSlug: organization.slug} : skipToken, + query: { + dataset: DiscoverDatasets.OURLOGS, + field: fields, + query: missingIds.length > 0 ? `id:[${missingIds.join(',')}]` : '', + project: selection.projects, + ...(selection.datetime.period + ? {statsPeriod: selection.datetime.period} + : { + start: selection.datetime.start ?? undefined, + end: selection.datetime.end ?? undefined, + }), + environment: selection.environments, + per_page: missingIds.length, + sampling: SAMPLING_MODE.HIGH_ACCURACY, + referrer: 'api.explore.logs-pinned', + }, + staleTime: 0, + }) + ); + + const {removePinnedRow} = logsPinning ?? {}; + + useEffect(() => { + if ( + !queryResult.data || + queryResult.data.meta?.dataScanned === 'partial' || + !queryResult.isSuccess || + !removePinnedRow + ) { + return; + } + + const foundIds = new Set( + queryResult.data.data.map(row => row[OurLogKnownFieldKey.ID]) + ); + + for (const id of missingIds) { + if (!foundIds.has(id)) { + removePinnedRow(id); + } + } + }, [queryResult.isSuccess, queryResult.data, missingIds, removePinnedRow]); + + return { + fetchedRows: queryResult.data?.data ?? [], + isPending: queryResult.isPending && missingIds.length > 0, + }; +} diff --git a/static/app/views/explore/logs/tables/logsInfiniteTable.tsx b/static/app/views/explore/logs/tables/logsInfiniteTable.tsx index dc8a2d33c78749..9145b75f61d91b 100644 --- a/static/app/views/explore/logs/tables/logsInfiniteTable.tsx +++ b/static/app/views/explore/logs/tables/logsInfiniteTable.tsx @@ -49,6 +49,7 @@ import { import {getDisplayTotalPayloadBytes} from 'sentry/views/explore/logs/getDisplayTotalPayloadBytes'; import {PinnedLogs} from 'sentry/views/explore/logs/pinning/PinnedLogs'; import {useLogsPinning} from 'sentry/views/explore/logs/pinning/useLogsPinning'; +import {usePinnedLogsQuery} from 'sentry/views/explore/logs/pinning/usePinnedLogsQuery'; import { FirstTableHeadCell, FloatingBackToTopContainer, @@ -446,6 +447,7 @@ export function LogsInfiniteTable({ }, []); const logsPinning = useLogsPinning(); + const pinnedLogsQuery = usePinnedLogsQuery({allRows: data, logsPinning}); const renderRow = useCallback( (dataRow: LogTableRowItem) => { @@ -531,7 +533,12 @@ export function LogsInfiniteTable({ /> )} {!isPending && logsPinning && ( - + )} Date: Wed, 3 Jun 2026 16:14:14 -0400 Subject: [PATCH 2/2] fix(ourlogs): Remove pinned logs in one update and send utc flag Replace removePinnedRow with removePinnedRows so invalidation removes all not-yet-available pinned ids in a single state update. Calling the single-id setter in a loop dropped removals: nuqs runs setState updaters at render, so each synchronous call saw the same committed state and only the last write survived. Include the utc flag in the pinned-log fetch for absolute date ranges so it matches the main logs query's time window and avoids spurious pin invalidation. Also fix pre-existing typecheck failures (LogFixture missing ORGANIZATION_ID, PinnedLogs wrapper row type) and a require-await lint error in the specs. Refs LOGS-781 Co-Authored-By: Claude --- .../explore/logs/pinning/PinnedLogs.spec.tsx | 9 +- .../logs/pinning/useLogsPinning.spec.tsx | 18 +++- .../explore/logs/pinning/useLogsPinning.tsx | 17 ++-- .../logs/pinning/usePinnedLogsQuery.spec.tsx | 83 +++++++++++++++++-- .../logs/pinning/usePinnedLogsQuery.tsx | 14 ++-- 5 files changed, 113 insertions(+), 28 deletions(-) diff --git a/static/app/views/explore/logs/pinning/PinnedLogs.spec.tsx b/static/app/views/explore/logs/pinning/PinnedLogs.spec.tsx index 6d21eee67551f5..8871893d0681e2 100644 --- a/static/app/views/explore/logs/pinning/PinnedLogs.spec.tsx +++ b/static/app/views/explore/logs/pinning/PinnedLogs.spec.tsx @@ -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[] = [ @@ -37,7 +40,7 @@ function PinnedLogsWrapper({ fetchedPinnedRows = [], isFetchingPinnedRows = false, }: { - fetchedPinnedRows?: LogTableRowItem[]; + fetchedPinnedRows?: OurLogsResponseItem[]; isFetchingPinnedRows?: boolean; }) { const logsPinning = useLogsPinning()!; @@ -60,7 +63,7 @@ function PinnedLogsWrapper({ function renderPinnedLogs( options: RenderOptions = {}, wrapperProps?: { - fetchedPinnedRows?: LogTableRowItem[]; + fetchedPinnedRows?: OurLogsResponseItem[]; isFetchingPinnedRows?: boolean; } ) { diff --git a/static/app/views/explore/logs/pinning/useLogsPinning.spec.tsx b/static/app/views/explore/logs/pinning/useLogsPinning.spec.tsx index 9220c8f96ae8d3..d515ff471a91e9 100644 --- a/static/app/views/explore/logs/pinning/useLogsPinning.spec.tsx +++ b/static/app/views/explore/logs/pinning/useLogsPinning.spec.tsx @@ -102,7 +102,7 @@ describe('useLogsPinning', () => { expect(result.current?.getPinnedRowIds()).toEqual([]); }); - it('removes the id from pinnedRows when removePinnedRow is called for a pinned id', () => { + 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'}}, @@ -110,10 +110,24 @@ describe('useLogsPinning', () => { }); act(() => { - result.current?.removePinnedRow('log-1'); + 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']); + }); }); }); diff --git a/static/app/views/explore/logs/pinning/useLogsPinning.tsx b/static/app/views/explore/logs/pinning/useLogsPinning.tsx index ba61bc5089f625..6e2c2be38df35c 100644 --- a/static/app/views/explore/logs/pinning/useLogsPinning.tsx +++ b/static/app/views/explore/logs/pinning/useLogsPinning.tsx @@ -9,7 +9,7 @@ export interface LogsPinning { clearPinnedRows: () => void; getPinnedRowIds: () => string[]; hasPinnedRow: (id: string) => boolean; - removePinnedRow: (id: string) => void; + removePinnedRows: (ids: string[]) => void; togglePinnedRow: (id: string) => void; } @@ -40,11 +40,14 @@ export function useLogsPinning(): LogsPinning | undefined { [setPinnedRowsList] ); - const removePinnedRow = useCallback( - (id: string) => { - setPinnedRowsList(pinnedRowsList.filter(previousId => previousId !== id)); + const removePinnedRows = useCallback( + (ids: string[]) => { + const idsToRemove = new Set(ids); + setPinnedRowsList(previous => + previous.filter(previousId => !idsToRemove.has(previousId)) + ); }, - [pinnedRowsList, setPinnedRowsList] + [setPinnedRowsList] ); const clearPinnedRows = useCallback(() => { @@ -56,10 +59,10 @@ export function useLogsPinning(): LogsPinning | undefined { clearPinnedRows, getPinnedRowIds, hasPinnedRow, - removePinnedRow, + removePinnedRows, togglePinnedRow, }), - [clearPinnedRows, getPinnedRowIds, hasPinnedRow, removePinnedRow, togglePinnedRow] + [clearPinnedRows, getPinnedRowIds, hasPinnedRow, removePinnedRows, togglePinnedRow] ); return logsPinningEnabled ? value : undefined; diff --git a/static/app/views/explore/logs/pinning/usePinnedLogsQuery.spec.tsx b/static/app/views/explore/logs/pinning/usePinnedLogsQuery.spec.tsx index 8d13e133f7dbb7..7baad540b272c4 100644 --- a/static/app/views/explore/logs/pinning/usePinnedLogsQuery.spec.tsx +++ b/static/app/views/explore/logs/pinning/usePinnedLogsQuery.spec.tsx @@ -22,7 +22,7 @@ function makeLogsPinning(pinnedIds: string[]): LogsPinning { clearPinnedRows: jest.fn(), getPinnedRowIds: jest.fn().mockReturnValue(pinnedIds), hasPinnedRow: jest.fn((id: string) => pinnedIds.includes(id)), - removePinnedRow: jest.fn(), + removePinnedRows: jest.fn(), togglePinnedRow: jest.fn(), }; } @@ -59,6 +59,7 @@ describe('usePinnedLogsQuery', () => { const logRow = LogFixture({ [OurLogKnownFieldKey.ID]: 'log-1', [OurLogKnownFieldKey.PROJECT_ID]: String(project.id), + [OurLogKnownFieldKey.ORGANIZATION_ID]: Number(organization.id), }); const logsPinning = makeLogsPinning(['log-1']); @@ -85,6 +86,7 @@ describe('usePinnedLogsQuery', () => { const missingLog = LogFixture({ [OurLogKnownFieldKey.ID]: 'log-missing', [OurLogKnownFieldKey.PROJECT_ID]: String(project.id), + [OurLogKnownFieldKey.ORGANIZATION_ID]: Number(organization.id), [OurLogKnownFieldKey.MESSAGE]: 'fetched log', }); @@ -121,7 +123,55 @@ describe('usePinnedLogsQuery', () => { expect(result.current.fetchedRows[0]?.[OurLogKnownFieldKey.ID]).toBe('log-missing'); }); - it('does not call removePinnedRow when the scan was only partial', async () => { + it('includes the utc flag when fetching with an absolute date range', async () => { + PageFiltersStore.onInitializeUrlState({ + projects: [parseInt(project.id, 10)], + environments: [], + datetime: { + period: null, + start: '2024-01-01T00:00:00', + end: '2024-01-02T00:00:00', + utc: true, + }, + }); + + const missingLog = LogFixture({ + [OurLogKnownFieldKey.ID]: 'log-missing', + [OurLogKnownFieldKey.PROJECT_ID]: String(project.id), + [OurLogKnownFieldKey.ORGANIZATION_ID]: Number(organization.id), + }); + + const eventsRequest = MockApiClient.addMockResponse({ + url: `/organizations/${organization.slug}/events/`, + method: 'GET', + body: { + data: [missingLog], + meta: {fields: {id: 'string'}, units: {}}, + }, + }); + + const logsPinning = makeLogsPinning(['log-missing']); + + renderHookWithProviders(() => usePinnedLogsQuery({allRows: [], logsPinning}), { + organization, + additionalWrapper: AdditionalWrapper, + }); + + await waitFor(() => { + expect(eventsRequest).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + query: expect.objectContaining({ + utc: true, + start: '2024-01-01T00:00:00', + end: '2024-01-02T00:00:00', + }), + }) + ); + }); + }); + + it('does not call removePinnedRows when the scan was only partial', async () => { MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/events/`, method: 'GET', @@ -142,20 +192,26 @@ describe('usePinnedLogsQuery', () => { expect(result.current.isPending).toBe(false); }); - expect(logsPinning.removePinnedRow).not.toHaveBeenCalled(); + expect(logsPinning.removePinnedRows).not.toHaveBeenCalled(); }); - it('calls removePinnedRow for ids not found in the API response', async () => { + it('calls removePinnedRows with every id not found in the API response', async () => { + const foundLog = LogFixture({ + [OurLogKnownFieldKey.ID]: 'log-found', + [OurLogKnownFieldKey.PROJECT_ID]: String(project.id), + [OurLogKnownFieldKey.ORGANIZATION_ID]: Number(organization.id), + }); + MockApiClient.addMockResponse({ url: `/organizations/${organization.slug}/events/`, method: 'GET', body: { - data: [], + data: [foundLog], meta: {fields: {id: 'string'}, units: {}}, }, }); - const logsPinning = makeLogsPinning(['log-gone']); + const logsPinning = makeLogsPinning(['log-gone-1', 'log-found', 'log-gone-2']); renderHookWithProviders(() => usePinnedLogsQuery({allRows: [], logsPinning}), { organization, @@ -163,14 +219,18 @@ describe('usePinnedLogsQuery', () => { }); await waitFor(() => { - expect(logsPinning.removePinnedRow).toHaveBeenCalledWith('log-gone'); + expect(logsPinning.removePinnedRows).toHaveBeenCalledWith([ + 'log-gone-1', + 'log-gone-2', + ]); }); }); - it('does not call removePinnedRow for ids that are found in the API response', async () => { + it('does not call removePinnedRows for ids that are found in the API response', async () => { const foundLog = LogFixture({ [OurLogKnownFieldKey.ID]: 'log-found', [OurLogKnownFieldKey.PROJECT_ID]: String(project.id), + [OurLogKnownFieldKey.ORGANIZATION_ID]: Number(organization.id), }); MockApiClient.addMockResponse({ @@ -190,7 +250,7 @@ describe('usePinnedLogsQuery', () => { }); await waitFor(() => { - expect(logsPinning.removePinnedRow).not.toHaveBeenCalled(); + expect(logsPinning.removePinnedRows).not.toHaveBeenCalled(); }); }); @@ -217,12 +277,17 @@ describe('usePinnedLogsQuery', () => { act(() => { resolveRequest({}); }); + + await waitFor(() => { + expect(result.current.isPending).toBe(false); + }); }); it('does not fetch when pinned ids are already in allRows', () => { const existingLog = LogFixture({ [OurLogKnownFieldKey.ID]: 'log-existing', [OurLogKnownFieldKey.PROJECT_ID]: String(project.id), + [OurLogKnownFieldKey.ORGANIZATION_ID]: Number(organization.id), }); const eventsRequest = MockApiClient.addMockResponse({ diff --git a/static/app/views/explore/logs/pinning/usePinnedLogsQuery.tsx b/static/app/views/explore/logs/pinning/usePinnedLogsQuery.tsx index 9cd5410f7e6b08..5c98df0c53d879 100644 --- a/static/app/views/explore/logs/pinning/usePinnedLogsQuery.tsx +++ b/static/app/views/explore/logs/pinning/usePinnedLogsQuery.tsx @@ -55,6 +55,7 @@ export function usePinnedLogsQuery({allRows, logsPinning}: PinnedLogsOptions) { : { start: selection.datetime.start ?? undefined, end: selection.datetime.end ?? undefined, + utc: selection.datetime.utc ?? undefined, }), environment: selection.environments, per_page: missingIds.length, @@ -65,14 +66,14 @@ export function usePinnedLogsQuery({allRows, logsPinning}: PinnedLogsOptions) { }) ); - const {removePinnedRow} = logsPinning ?? {}; + const {removePinnedRows} = logsPinning ?? {}; useEffect(() => { if ( !queryResult.data || queryResult.data.meta?.dataScanned === 'partial' || !queryResult.isSuccess || - !removePinnedRow + !removePinnedRows ) { return; } @@ -81,12 +82,11 @@ export function usePinnedLogsQuery({allRows, logsPinning}: PinnedLogsOptions) { queryResult.data.data.map(row => row[OurLogKnownFieldKey.ID]) ); - for (const id of missingIds) { - if (!foundIds.has(id)) { - removePinnedRow(id); - } + const idsToRemove = missingIds.filter(id => !foundIds.has(id)); + if (idsToRemove.length > 0) { + removePinnedRows(idsToRemove); } - }, [queryResult.isSuccess, queryResult.data, missingIds, removePinnedRow]); + }, [queryResult.isSuccess, queryResult.data, missingIds, removePinnedRows]); return { fetchedRows: queryResult.data?.data ?? [],