Skip to content

Commit a7cd4d8

Browse files
JoshuaKGoldbergJoshuaKGoldberg
authored
fix(logs): Prevent cell action menu clicks from toggling row visibility (#111717)
The dropdown menu renders in a React portal, so its DOM lives outside the row. This adds a DOM containment check: before toggling, verify `event.currentTarget.contains(event.target)`. Portal-originating events fail this check since their DOM subtree is separate from the row's. Fixes LOGS-638 Made with [Cursor](https://cursor.com)
1 parent 1e1d235 commit a7cd4d8

File tree

2 files changed

+123
-66
lines changed

2 files changed

+123
-66
lines changed

‎static/app/views/explore/logs/tables/logsTableRow.spec.tsx‎

Lines changed: 108 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -213,17 +213,15 @@ describe('logsTableRow', () => {
213213
jest.useFakeTimers();
214214
expect(rowDetailsMock).toHaveBeenCalledTimes(0);
215215
render(
216-
<ProviderWrapper>
217-
<LogRowContent
218-
dataRow={rowData}
219-
highlightTerms={[]}
220-
meta={LogFixtureMeta(rowData)}
221-
sharedHoverTimeoutRef={
222-
{current: null} as React.MutableRefObject<NodeJS.Timeout | null>
223-
}
224-
/>
225-
</ProviderWrapper>,
226-
{organization, initialRouterConfig}
216+
<LogRowContent
217+
dataRow={rowData}
218+
highlightTerms={[]}
219+
meta={LogFixtureMeta(rowData)}
220+
sharedHoverTimeoutRef={
221+
{current: null} as React.MutableRefObject<NodeJS.Timeout | null>
222+
}
223+
/>,
224+
{organization, initialRouterConfig, additionalWrapper: ProviderWrapper}
227225
);
228226

229227
expect(screen.queryByLabelText('Toggle trace details')).not.toBeInTheDocument(); // Fake button
@@ -247,19 +245,17 @@ describe('logsTableRow', () => {
247245

248246
it('renders row details', async () => {
249247
render(
250-
<ProviderWrapper>
251-
<LogRowContent
252-
dataRow={rowData}
253-
highlightTerms={[]}
254-
meta={LogFixtureMeta(rowData)}
255-
sharedHoverTimeoutRef={
256-
{
257-
current: null,
258-
} as React.MutableRefObject<NodeJS.Timeout | null>
259-
}
260-
/>
261-
</ProviderWrapper>,
262-
{organization, initialRouterConfig}
248+
<LogRowContent
249+
dataRow={rowData}
250+
highlightTerms={[]}
251+
meta={LogFixtureMeta(rowData)}
252+
sharedHoverTimeoutRef={
253+
{
254+
current: null,
255+
} as React.MutableRefObject<NodeJS.Timeout | null>
256+
}
257+
/>,
258+
{organization, initialRouterConfig, additionalWrapper: ProviderWrapper}
263259
);
264260

265261
// Check that the log body and timestamp are rendered
@@ -349,19 +345,21 @@ describe('logsTableRow', () => {
349345

350346
it('shows a link when hovering over code file path in the table', async () => {
351347
render(
352-
<ProviderWrapper>
353-
<LogRowContent
354-
dataRow={rowDataWithCodeFilePath}
355-
highlightTerms={[]}
356-
meta={LogFixtureMeta(rowDataWithCodeFilePath)}
357-
sharedHoverTimeoutRef={
358-
{
359-
current: null,
360-
} as React.MutableRefObject<NodeJS.Timeout | null>
361-
}
362-
/>
363-
</ProviderWrapper>,
364-
{organization, initialRouterConfig: initialRouterConfigWithCodeFilePath}
348+
<LogRowContent
349+
dataRow={rowDataWithCodeFilePath}
350+
highlightTerms={[]}
351+
meta={LogFixtureMeta(rowDataWithCodeFilePath)}
352+
sharedHoverTimeoutRef={
353+
{
354+
current: null,
355+
} as React.MutableRefObject<NodeJS.Timeout | null>
356+
}
357+
/>,
358+
{
359+
organization,
360+
initialRouterConfig: initialRouterConfigWithCodeFilePath,
361+
additionalWrapper: ProviderWrapper,
362+
}
365363
);
366364

367365
// Expand the row to show the attributes
@@ -430,19 +428,17 @@ describe('logsTableRow', () => {
430428
});
431429

432430
render(
433-
<ProviderWrapper>
434-
<LogRowContent
435-
dataRow={rowData}
436-
highlightTerms={[]}
437-
meta={LogFixtureMeta(rowData)}
438-
sharedHoverTimeoutRef={
439-
{
440-
current: null,
441-
} as React.MutableRefObject<NodeJS.Timeout | null>
442-
}
443-
/>
444-
</ProviderWrapper>,
445-
{organization, initialRouterConfig}
431+
<LogRowContent
432+
dataRow={rowData}
433+
highlightTerms={[]}
434+
meta={LogFixtureMeta(rowData)}
435+
sharedHoverTimeoutRef={
436+
{
437+
current: null,
438+
} as React.MutableRefObject<NodeJS.Timeout | null>
439+
}
440+
/>,
441+
{organization, initialRouterConfig, additionalWrapper: ProviderWrapper}
446442
);
447443

448444
// Expand the row to show the action buttons
@@ -489,6 +485,53 @@ describe('logsTableRow', () => {
489485
expect(parsedData).not.toHaveProperty('sentry.item_id');
490486
});
491487

488+
it('does not toggle row when clicking cell action menu items', async () => {
489+
const mockWriteText = jest.fn().mockResolvedValue(undefined);
490+
Object.defineProperty(window.navigator, 'clipboard', {
491+
value: {
492+
writeText: mockWriteText,
493+
},
494+
writable: true,
495+
});
496+
497+
render(
498+
<LogRowContent
499+
dataRow={rowData}
500+
highlightTerms={[]}
501+
meta={LogFixtureMeta(rowData)}
502+
sharedHoverTimeoutRef={
503+
{
504+
current: null,
505+
} as React.MutableRefObject<NodeJS.Timeout | null>
506+
}
507+
/>,
508+
{organization, initialRouterConfig, additionalWrapper: ProviderWrapper}
509+
);
510+
511+
const logTableRow = await screen.findByTestId('log-table-row');
512+
await userEvent.click(logTableRow);
513+
514+
await waitFor(() => {
515+
expect(rowDetailsMock).toHaveBeenCalledTimes(1);
516+
});
517+
518+
// Row is expanded - verify details are visible
519+
expect(await screen.findByRole('button', {name: 'Copy as JSON'})).toBeInTheDocument();
520+
521+
// Open the ellipsis context menu on a cell
522+
const actionsButton = screen.getAllByRole('button', {name: 'Actions'})[0]!;
523+
await userEvent.click(actionsButton);
524+
525+
// Click "Copy to clipboard" in the dropdown menu
526+
const copyItem = await screen.findByRole('menuitemradio', {
527+
name: 'Copy to clipboard',
528+
});
529+
await userEvent.click(copyItem);
530+
531+
// Row should still be expanded - the cell action should not toggle visibility
532+
expect(screen.getByRole('button', {name: 'Copy as JSON'})).toBeInTheDocument();
533+
});
534+
492535
it('renders fields with data scrubbing meta information', async () => {
493536
const traceItemMock = MockApiClient.addMockResponse({
494537
url: `/projects/${organization.slug}/${project.slug}/trace-items/${rowDataWithScrubbedFields[OurLogKnownFieldKey.ID]}/`,
@@ -531,19 +574,21 @@ describe('logsTableRow', () => {
531574
});
532575

533576
render(
534-
<ProviderWrapper>
535-
<LogRowContent
536-
dataRow={rowDataWithScrubbedFields}
537-
highlightTerms={[]}
538-
meta={LogFixtureMeta(rowDataWithScrubbedFields)}
539-
sharedHoverTimeoutRef={
540-
{
541-
current: null,
542-
} as React.MutableRefObject<NodeJS.Timeout | null>
543-
}
544-
/>
545-
</ProviderWrapper>,
546-
{organization, initialRouterConfig: initialRouterConfigWithScrubbedFields}
577+
<LogRowContent
578+
dataRow={rowDataWithScrubbedFields}
579+
highlightTerms={[]}
580+
meta={LogFixtureMeta(rowDataWithScrubbedFields)}
581+
sharedHoverTimeoutRef={
582+
{
583+
current: null,
584+
} as React.MutableRefObject<NodeJS.Timeout | null>
585+
}
586+
/>,
587+
{
588+
organization,
589+
initialRouterConfig: initialRouterConfigWithScrubbedFields,
590+
additionalWrapper: ProviderWrapper,
591+
}
547592
);
548593

549594
const logTableRow = await screen.findByTestId('log-table-row');

‎static/app/views/explore/logs/tables/logsTableRow.tsx‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -184,10 +184,22 @@ export const LogRowContent = memo(function LogRowContent({
184184
}
185185

186186
function onPointerUp(event: SyntheticEvent) {
187-
if (event.target instanceof Element && isInsideButton(event.target)) {
188-
// do not expand the context menu if you clicked a button
189-
return;
187+
// do not expand the context menu if...
188+
if (event.target instanceof Element) {
189+
// ... you clicked a button
190+
if (isInsideButton(event.target)) {
191+
return;
192+
}
193+
194+
// ... you clicked outside the row (e.g. a portal button)
195+
if (
196+
!(event.currentTarget instanceof Node) ||
197+
!event.currentTarget.contains(event.target)
198+
) {
199+
return;
200+
}
190201
}
202+
191203
if (window.getSelection()?.toString() === '') {
192204
toggleExpanded();
193205
}

0 commit comments

Comments
 (0)