Skip to content

fix(ourlogs): Add drawer QP and fix scroll #103979

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 25, 2025
Merged

fix(ourlogs): Add drawer QP and fix scroll #103979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 38 additions & 5 deletions static/app/components/events/metrics/metricsSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {useCallback, useRef} from 'react';
import {useCallback, useEffect, useRef} from 'react';

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

Expand All @@ -12,8 +12,11 @@ import type {Event} from 'sentry/types/event';
import type {Group} from 'sentry/types/group';
import type {Project} from 'sentry/types/project';
import {trackAnalytics} from 'sentry/utils/analytics';
import {useLocation} from 'sentry/utils/useLocation';
import {useNavigate} from 'sentry/utils/useNavigate';
import useOrganization from 'sentry/utils/useOrganization';
import {TraceItemAttributeProvider} from 'sentry/views/explore/contexts/traceItemAttributeContext';
import {METRICS_DRAWER_QUERY_PARAM} from 'sentry/views/explore/metrics/constants';
import {MetricsSamplesTable} from 'sentry/views/explore/metrics/metricInfoTabs/metricsSamplesTable';
import {canUseMetricsUI} from 'sentry/views/explore/metrics/metricsFlags';
import {TraceItemDataset} from 'sentry/views/explore/types';
Expand Down Expand Up @@ -68,6 +71,8 @@ function MetricsSectionContent({
traceId: string;
}) {
const organization = useOrganization();
const navigate = useNavigate();
const location = useLocation();
const {openDrawer} = useDrawer();
const viewAllButtonRef = useRef<HTMLButtonElement>(null);
const {result, error} = useMetricsIssueSection({traceId});
Expand All @@ -81,6 +86,24 @@ function MetricsSectionContent({
trackAnalytics('metrics.issue_details.drawer_opened', {
organization,
});

navigate(
{
...location,
query: {
...location.query,
[METRICS_DRAWER_QUERY_PARAM]: 'true',
},
},
{replace: true}
);
},
[navigate, location, organization]
);

useEffect(() => {
const shouldOpenDrawer = location.query[METRICS_DRAWER_QUERY_PARAM] === 'true';
if (shouldOpenDrawer) {
openDrawer(
() => (
<TraceViewMetricsProviderWrapper traceSlug={traceId}>
Expand All @@ -99,14 +122,24 @@ function MetricsSectionContent({
const viewAllButton = viewAllButtonRef.current;
return !viewAllButton?.contains(element);
},
onClose: () => {
navigate(
{
...location,
query: {
...location.query,
[METRICS_DRAWER_QUERY_PARAM]: undefined,
},
},
{replace: true}
);
},
}
);
},
[group, event, project, openDrawer, organization, traceId]
);
}
}, [location.query, traceId, group, event, project, openDrawer, navigate, location]);

if (!result.data || result.data.length === 0 || error) {
// Don't show the metrics section if there are no metrics
return null;
}

Expand Down
8 changes: 7 additions & 1 deletion static/app/components/events/ourlogs/ourlogsDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ interface LogIssueDrawerProps {
event: Event;
group: Group;
project: Project;
additionalData?: {
event?: Event;
scrollToDisabled?: boolean;
};
embeddedOptions?: {
openWithExpandedIds?: string[];
};
Expand All @@ -53,6 +57,7 @@ export function OurlogsDrawer({
project,
group,
embeddedOptions,
additionalData: propAdditionalData,
}: LogIssueDrawerProps) {
const organization = useOrganization();
const setLogsQuery = useSetQueryParamsQuery();
Expand Down Expand Up @@ -81,8 +86,9 @@ export function OurlogsDrawer({
const additionalData = useMemo(
() => ({
event,
scrollToDisabled: propAdditionalData?.scrollToDisabled,
}),
[event]
[event, propAdditionalData?.scrollToDisabled]
);

const exploreUrl = useMemo(() => {
Expand Down
65 changes: 53 additions & 12 deletions static/app/components/events/ourlogs/ourlogsSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {useCallback, useRef} from 'react';
import {useCallback, useEffect, useRef} from 'react';
import styled from '@emotion/styled';

import {Button} from 'sentry/components/core/button';
Expand All @@ -11,13 +11,16 @@ import type {Group} from 'sentry/types/group';
import type {Project} from 'sentry/types/project';
import {trackAnalytics} from 'sentry/utils/analytics';
import {LogsAnalyticsPageSource} from 'sentry/utils/analytics/logsAnalyticsEvent';
import {useLocation} from 'sentry/utils/useLocation';
import {useNavigate} from 'sentry/utils/useNavigate';
import useOrganization from 'sentry/utils/useOrganization';
import {TableBody} from 'sentry/views/explore/components/table';
import {
LogsPageDataProvider,
useLogsPageDataQueryResult,
} from 'sentry/views/explore/contexts/logs/logsPageData';
import {TraceItemAttributeProvider} from 'sentry/views/explore/contexts/traceItemAttributeContext';
import {LOGS_DRAWER_QUERY_PARAM} from 'sentry/views/explore/logs/constants';
import {LogsQueryParamsProvider} from 'sentry/views/explore/logs/logsQueryParamsProvider';
import {LogRowContent} from 'sentry/views/explore/logs/tables/logsTableRow';
import {useQueryParamsSearch} from 'sentry/views/explore/queryParams/context';
Expand Down Expand Up @@ -58,6 +61,8 @@ function OurlogsSectionContent({
project: Project;
}) {
const organization = useOrganization();
const navigate = useNavigate();
const location = useLocation();
const feature = organization.features.includes('ourlogs-enabled');
const tableData = useLogsPageDataQueryResult();
const logsSearch = useQueryParamsSearch();
Expand All @@ -73,6 +78,34 @@ function OurlogsSectionContent({
trackAnalytics('logs.issue_details.drawer_opened', {
organization,
});

navigate(
{
...location,
query: {
...location.query,
[LOGS_DRAWER_QUERY_PARAM]: 'true',
...(expandedLogId && {expandedLogId}),
},
},
{replace: true}
);
},
[navigate, location, organization]
);

const onEmbeddedRowClick = useCallback(
(logItemId: string, clickEvent: React.MouseEvent) => {
onOpenLogsDrawer(clickEvent, logItemId);
},
[onOpenLogsDrawer]
);

useEffect(() => {
const shouldOpenDrawer = location.query[LOGS_DRAWER_QUERY_PARAM] === 'true';
if (shouldOpenDrawer && traceId) {
const expandedLogId = location.query.expandedLogId as string | undefined;

openDrawer(
() => (
<LogsQueryParamsProvider
Expand All @@ -89,6 +122,10 @@ function OurlogsSectionContent({
embeddedOptions={
expandedLogId ? {openWithExpandedIds: [expandedLogId]} : undefined
}
additionalData={{
event,
scrollToDisabled: !!expandedLogId,
}}
/>
</TraceItemAttributeProvider>
</LogsPageDataProvider>
Expand All @@ -97,23 +134,27 @@ function OurlogsSectionContent({
{
ariaLabel: 'logs drawer',
drawerKey: 'logs-issue-drawer',

shouldCloseOnInteractOutside: element => {
const viewAllButton = viewAllButtonRef.current;
return !viewAllButton?.contains(element);
},
onClose: () => {
navigate(
{
...location,
query: {
...location.query,
[LOGS_DRAWER_QUERY_PARAM]: undefined,
expandedLogId: undefined,
},
},
{replace: true}
);
},
}
);
},
[group, event, project, openDrawer, organization, traceId]
Comment on lines 140 to -108
Copy link

@sentry sentry bot Nov 25, 2025

Choose a reason for hiding this comment

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

Bug: Race condition in onOpenLogsDrawer can cause expandedLogId to persist in URL due to stale location.query after drawer close.
Severity: HIGH | Confidence: High

🔍 Detailed Analysis

When onOpenLogsDrawer is called after onClose has been triggered but before the location object has fully updated to reflect expandedLogId: undefined, the ...location.query spread will include the stale expandedLogId from the previous navigation. This occurs if a user closes the drawer and immediately clicks "View more" again. The drawer then opens with scrollToDisabled: true even though no specific log was selected, breaking the intended scroll behavior for the pseudo event.

💡 Suggested Fix

Modify onOpenLogsDrawer to explicitly set expandedLogId: undefined in the new query object when no expandedLogId is passed, instead of relying on ...location.query to implicitly remove it. This ensures the parameter is cleared regardless of location object staleness.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/components/events/ourlogs/ourlogsSection.tsx#L104-L108

Potential issue: When `onOpenLogsDrawer` is called after `onClose` has been triggered
but before the `location` object has fully updated to reflect `expandedLogId:
undefined`, the `...location.query` spread will include the stale `expandedLogId` from
the previous navigation. This occurs if a user closes the drawer and immediately clicks
"View more" again. The drawer then opens with `scrollToDisabled: true` even though no
specific log was selected, breaking the intended scroll behavior for the pseudo event.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3557864

);

const onEmbeddedRowClick = useCallback(
(logItemId: string, clickEvent: React.MouseEvent) => {
onOpenLogsDrawer(clickEvent, logItemId);
},
[onOpenLogsDrawer]
);
}
}, [location.query, traceId, group, event, project, openDrawer, navigate, location]);
Copy link
Contributor

@cursor cursor bot Nov 25, 2025

Choose a reason for hiding this comment

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

Bug: Drawer opens even when feature flag is disabled

The useEffect that opens the logs drawer checks shouldOpenDrawer && traceId but doesn't check the ourlogs-enabled feature flag. The feature flag check at line 158 happens after the effect is defined, but React hooks run regardless of what the component returns. This means if someone shares a URL with logsDrawer=true to an organization that doesn't have ourlogs-enabled, the drawer will still open, bypassing the feature gate. The effect's condition needs to include feature (e.g., shouldOpenDrawer && traceId && feature).

Fix in Cursor Fix in Web

if (!feature) {
return null;
}
Expand Down
6 changes: 6 additions & 0 deletions static/app/views/explore/logs/constants.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ export const LOGS_INSTRUCTIONS_URL =

export const LOGS_FILTER_KEY_SECTIONS: FilterKeySection[] = [LOGS_FILTERS];

/**
* Query parameter key for controlling the logs drawer state.
* When this parameter is set to 'true', the logs drawer should open automatically.
*/
export const LOGS_DRAWER_QUERY_PARAM = 'logsDrawer';

export const VIRTUAL_STREAMED_INTERVAL_MS = 250;
export const MINIMUM_INFINITE_SCROLL_FETCH_COOLDOWN_MS = 1000;

Expand Down
36 changes: 29 additions & 7 deletions static/app/views/explore/logs/tables/logsInfiniteTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type {CSSProperties} from 'react';
import type {CSSProperties, RefObject} from 'react';
import {Fragment, useCallback, useEffect, useMemo, useRef, useState} from 'react';
import {useTheme} from '@emotion/react';
import styled from '@emotion/styled';
Expand Down Expand Up @@ -74,6 +74,7 @@ import {EmptyStateText} from 'sentry/views/explore/tables/tracesTable/styles';
type LogsTableProps = {
additionalData?: {
event?: Event;
scrollToDisabled?: boolean;
};
allowPagination?: boolean;
embedded?: boolean;
Expand Down Expand Up @@ -132,6 +133,7 @@ export function LogsInfiniteTable({
} = useLogsPageDataQueryResult();

const baseData = localOnlyItemFilters?.filteredItems ?? originalData;
const baseDataLength = useBox(baseData.length);

const pseudoRowIndex = useMemo(() => {
if (
Expand All @@ -149,7 +151,7 @@ export function LogsInfiniteTable({
row =>
isRegularLogResponseItem(row) && getLogRowTimestampMillis(row) < eventTimestamp
);
return index === -1 ? baseData.length : index; // If the event is older than all the data, add it to the end.
return index === -1 ? -2 : index; // If the event is older than all the data, add it to the end with a sentinel value of -2. This causes the useEffect to not continously add it.
}, [additionalData, baseData, isPending, isError]);

const data: LogTableRowItem[] = useMemo(() => {
Expand All @@ -165,16 +167,18 @@ export function LogsInfiniteTable({
}

const newData: LogTableRowItem[] = [...baseData];
const newSelectedIndex =
pseudoRowIndex === -2 ? baseDataLength.current : pseudoRowIndex;
newData.splice(
pseudoRowIndex,
newSelectedIndex,
0,
createPseudoLogResponseItem(
additionalData.event,
additionalData.event.projectID || ''
)
);
return newData;
}, [baseData, additionalData, isPending, isError, pseudoRowIndex]);
}, [baseData, additionalData, isPending, isError, pseudoRowIndex, baseDataLength]);

// Calculate quantized start and end times for replay links
const {logStart, logEnd} = useMemo(() => {
Expand Down Expand Up @@ -280,15 +284,27 @@ export function LogsInfiniteTable({
);

useEffect(() => {
if (pseudoRowIndex !== -1 && scrollContainer?.current) {
if (
pseudoRowIndex !== -1 &&
scrollContainer?.current &&
!additionalData?.scrollToDisabled
) {
setTimeout(() => {
containerVirtualizer.scrollToIndex(pseudoRowIndex, {
const scrollToIndex =
pseudoRowIndex === -2 ? baseDataLength.current : pseudoRowIndex;
containerVirtualizer.scrollToIndex(scrollToIndex, {
behavior: 'smooth',
align: 'center',
});
}, 100);
}
}, [pseudoRowIndex, containerVirtualizer, scrollContainer]);
}, [
pseudoRowIndex,
containerVirtualizer,
scrollContainer,
baseDataLength,
additionalData?.scrollToDisabled,
]);

const hasReplay = !!embeddedOptions?.replay;

Expand Down Expand Up @@ -773,3 +789,9 @@ function BackToTopButton({
</Button>
);
}

function useBox<T>(value: T): RefObject<T> {
const box = useRef(value);
box.current = value;
return box;
}
6 changes: 6 additions & 0 deletions static/app/views/explore/metrics/constants.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,3 +179,9 @@ export const DEFAULT_YAXIS_BY_TYPE: Record<string, string> = {
distribution: 'p75',
gauge: 'avg',
};

/**
* Query parameter key for controlling the metrics drawer state.
* When this parameter is set to 'true', the metrics drawer should open automatically.
*/
export const METRICS_DRAWER_QUERY_PARAM = 'metricsDrawer';
Loading