Skip to content

This hides the filter actions buttons (but not copy json) when embedded. #1

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+100 −59
Open

This hides the filter actions buttons (but not copy json) when embedded. #1

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
5 changes: 5 additions & 0 deletions static/app/views/explore/contexts/logs/logsPageParams.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,11 @@ export function useLogsLimitToTraceId() {
return limitToTraceId;
}

export function useLogsIsTableFrozen() {
const {isTableFrozen} = useLogsPageParams();
return isTableFrozen;
}

export function useSetLogsCursor() {
const setPageParams = useSetLogsPageParams();
const {setCursorForFrozenPages, isTableFrozen} = useLogsPageParams();
Expand Down
154 changes: 95 additions & 59 deletions static/app/views/explore/logs/tables/logsTableRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import {trackAnalytics} from 'sentry/utils/analytics';
import type {TableDataRow} from 'sentry/utils/discover/discoverQuery';
import type {EventsMetaType} from 'sentry/utils/discover/eventView';
import {FieldValueType} from 'sentry/utils/fields';
import type {UseApiQueryResult} from 'sentry/utils/queryClient';
import type RequestError from 'sentry/utils/requestError/requestError';
import useCopyToClipboard from 'sentry/utils/useCopyToClipboard';
import {useLocation} from 'sentry/utils/useLocation';
import useOrganization from 'sentry/utils/useOrganization';
Expand All @@ -33,7 +35,9 @@ import {
useLogsAddSearchFilter,
useLogsAnalyticsPageSource,
useLogsFields,
useLogsIsTableFrozen,
} from 'sentry/views/explore/contexts/logs/logsPageParams';
import type {TraceItemDetailsResponse} from 'sentry/views/explore/hooks/useTraceItemDetails';
import {
DEFAULT_TRACE_ITEM_HOVER_TIMEOUT,
DEFAULT_TRACE_ITEM_HOVER_TIMEOUT_WITH_AUTO_REFRESH,
Expand Down Expand Up @@ -368,7 +372,6 @@ function LogRowDetails({
}) {
const location = useLocation();
const organization = useOrganization();
const addSearchFilter = useLogsAddSearchFilter();
const project = useProjectFromId({
project_id: '' + dataRow[OurLogKnownFieldKey.PROJECT_ID],
});
Expand All @@ -383,25 +386,14 @@ function LogRowDetails({
typeof severityText === 'string' ? severityText : null
);
const missingLogId = !dataRow[OurLogKnownFieldKey.ID];
const {data, isPending, isError} = useExploreLogsTableRow({
const fullLogDataResult = useExploreLogsTableRow({
Copy link
Owner Author

Choose a reason for hiding this comment

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

Test this

Copy link
Owner Author

Choose a reason for hiding this comment

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

/propel review

Copy link
Owner Author

Choose a reason for hiding this comment

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

/propel review

Copy link
Owner Author

Choose a reason for hiding this comment

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

/propel review

Copy link
Owner Author

Choose a reason for hiding this comment

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

/propel review

Copy link
Owner Author

Choose a reason for hiding this comment

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

/propel review

Copy link
Owner Author

Choose a reason for hiding this comment

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

/propel review

Copy link
Owner Author

Choose a reason for hiding this comment

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

/propel review

Copy link
Owner Author

Choose a reason for hiding this comment

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

/propel review

Copy link

Choose a reason for hiding this comment

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

This pull request effectively addresses the stated goal of hiding filter action buttons when the logs table is embedded, while keeping the 'Copy as JSON' functionality available. The refactoring into LogRowDetailsFilterActions and LogRowDetailsActions components improves modularity and readability.

I agree with the suggestions made by propel-test-bot[bot]. Renaming showFilterButtons to canFilter provides better semantic clarity, and using null instead of an empty <span> for conditional rendering is a good practice for accessibility and cleaner JSX.

Here are the suggested changes:

Suggested change
const fullLogDataResult = useExploreLogsTableRow({
const canFilter = !isTableFrozen;
Suggested change
const fullLogDataResult = useExploreLogsTableRow({
{canFilter ? (
<LogRowDetailsFilterActions tableDataRow={tableDataRow} />
) : null}

⚡ Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

logId: String(dataRow[OurLogKnownFieldKey.ID] ?? ''),
projectId: String(dataRow[OurLogKnownFieldKey.PROJECT_ID] ?? ''),
traceId: String(dataRow[OurLogKnownFieldKey.TRACE_ID] ?? ''),
enabled: !missingLogId,
});

const {onClick: betterCopyToClipboard} = useCopyToClipboard({
Copy link
Owner Author

Choose a reason for hiding this comment

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

Let's test it again

Copy link

Choose a reason for hiding this comment

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

Understood. If you've made updates based on the previous suggestions or have any new questions, please let me know. I'm ready to review the changes when you are.

Copy link
Owner Author

Choose a reason for hiding this comment

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

/propel add this to knowledge base

Copy link
Owner Author

Choose a reason for hiding this comment

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

/propel add this to knowledge base

Copy link
Owner Author

Choose a reason for hiding this comment

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

/propel add this to knowledge base

Copy link

Choose a reason for hiding this comment

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

/propel add to knowledge base that we should never use useCopyToClipboard function

Copy link

Choose a reason for hiding this comment

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

I understand you'd like to add a rule to a knowledge base stating that the useCopyToClipboard function should never be used. As an AI, I don't have the ability to directly modify or add entries to your organization's knowledge base. My function is to provide code review feedback, answer questions, and offer suggestions based on the provided context.

If you can provide more details on why useCopyToClipboard should be avoided (e.g., if it's deprecated, has known issues, or there's a preferred alternative), I can help by providing context-aware suggestions or explanations in future code reviews. This could indirectly contribute to your team's best practices.

Copy link
Owner Author

@jasonyuezhang jasonyuezhang Aug 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

/propel help me review code again.

Copy link

Choose a reason for hiding this comment

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

Hello! I'm happy to help review your code again.

Your pull request effectively addresses the goal of hiding filter action buttons when the logs table is embedded, while retaining the 'Copy as JSON' functionality. The refactoring into LogRowDetailsFilterActions and LogRowDetailsActions components is a good step towards improving modularity and readability.

I noticed that the previous suggestions regarding variable naming and conditional rendering haven't been applied yet. These are still relevant and would further enhance the clarity and accessibility of your code:

  1. Rename showFilterButtons to canFilter: This provides a more semantically accurate name for the variable, indicating its purpose more clearly.
  2. Replace empty <span> with null: When conditionally rendering components in React, returning null is generally preferred over an empty <span> for better accessibility and cleaner JSX.

Here are the committable suggestions for these improvements:

Suggested change
const {onClick: betterCopyToClipboard} = useCopyToClipboard({
const canFilter = !isTableFrozen;
Suggested change
const {onClick: betterCopyToClipboard} = useCopyToClipboard({
{canFilter ? (
<LogRowDetailsFilterActions tableDataRow={tableDataRow} />
) : null}

Please let me know if you have any other questions or if you'd like me to review any further changes!

⚡ Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Copy link

Choose a reason for hiding this comment

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

Hello! I'd be happy to help you review the code. This pull request aims to hide filter action buttons when the logs table is embedded, while retaining the "Copy as JSON" functionality. The refactoring into LogRowDetailsFilterActions and LogRowDetailsActions components is a good step towards improving modularity and readability.

I've reviewed the changes and the previous comments. The suggestions from the propel-test-bot[bot] are valid and will improve the code's clarity and semantics. Let's incorporate those:

  1. Rename showFilterButtons to canFilter: This change makes the variable name more descriptive of its purpose.
  2. Replace <span /> with null for conditional rendering: Using null is a more idiomatic and semantically correct way to render nothing in React, improving accessibility and code cleanliness.

Regarding the useCopyToClipboard discussion, I understand there might be a preference or policy against its use. As an AI, I can't directly update your knowledge base. However, I can confirm that within the context of this PR, the useCopyToClipboard hook is being used correctly to provide the "Copy as JSON" functionality. If there's an alternative preferred method for copying to the clipboard within your organization, it would be a good idea to update this implementation accordingly in a separate change or as part of a broader refactor.

Overall, the changes are well-structured and achieve the stated goal. Once the suggested improvements are applied, the code will be even cleaner.

Here are the committable suggestions for the improvements mentioned:

Suggested change
const {onClick: betterCopyToClipboard} = useCopyToClipboard({
const canFilter = !isTableFrozen;
Suggested change
const {onClick: betterCopyToClipboard} = useCopyToClipboard({
{canFilter ? (
<LogRowDetailsFilterActions tableDataRow={tableDataRow} />
) : null}

⚡ Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

text: isPending || isError ? '' : ourlogToJson(data),
onCopy: () => {
trackAnalytics('logs.table.row_copied_as_json', {
log_id: String(dataRow[OurLogKnownFieldKey.ID]),
organization,
});
},

successMessage: t('Copied!'),
errorMessage: t('Failed to copy'),
});
const {data, isPending, isError} = fullLogDataResult;

const theme = useTheme();
const logColors = getLogColors(level, theme);
Expand Down Expand Up @@ -481,53 +473,97 @@ function LogRowDetails({
flexDirection: 'row',
}}
>
<LogDetailTableActionsButtonBar>
<Button
priority="link"
size="sm"
borderless
onClick={() => {
addSearchFilter({
key: OurLogKnownFieldKey.MESSAGE,
value: dataRow[OurLogKnownFieldKey.MESSAGE],
});
}}
>
<IconAdd size="md" style={{paddingRight: space(0.5)}} />
{t('Add to filter')}
</Button>
<Button
priority="link"
size="sm"
borderless
onClick={() => {
addSearchFilter({
key: OurLogKnownFieldKey.MESSAGE,
value: dataRow[OurLogKnownFieldKey.MESSAGE],
negated: true,
});
}}
>
<IconSubtract size="md" style={{paddingRight: space(0.5)}} />
{t('Exclude from filter')}
</Button>
</LogDetailTableActionsButtonBar>

<LogDetailTableActionsButtonBar>
<Button
priority="link"
size="sm"
borderless
onClick={() => {
betterCopyToClipboard();
}}
>
<IconJson size="md" style={{paddingRight: space(0.5)}} />
{t('Copy as JSON')}
</Button>
</LogDetailTableActionsButtonBar>
<LogRowDetailsActions
fullLogDataResult={fullLogDataResult}
tableDataRow={dataRow}
/>
</LogDetailTableActionsCell>
)}
</DetailsWrapper>
);
}

function LogRowDetailsFilterActions({tableDataRow}: {tableDataRow: OurLogsResponseItem}) {
const addSearchFilter = useLogsAddSearchFilter();
return (
<LogDetailTableActionsButtonBar>
<Button
priority="link"
size="sm"
borderless
onClick={() => {
addSearchFilter({
key: OurLogKnownFieldKey.MESSAGE,
value: tableDataRow[OurLogKnownFieldKey.MESSAGE],
});
}}
>
<IconAdd size="md" style={{paddingRight: space(0.5)}} />
{t('Add to filter')}
</Button>
<Button
priority="link"
size="sm"
borderless
onClick={() => {
addSearchFilter({
key: OurLogKnownFieldKey.MESSAGE,
value: tableDataRow[OurLogKnownFieldKey.MESSAGE],
negated: true,
});
}}
>
<IconSubtract size="md" style={{paddingRight: space(0.5)}} />
{t('Exclude from filter')}
</Button>
</LogDetailTableActionsButtonBar>
);
}

function LogRowDetailsActions({
fullLogDataResult,
tableDataRow,
}: {
fullLogDataResult: UseApiQueryResult<TraceItemDetailsResponse, RequestError>;
tableDataRow: OurLogsResponseItem;
}) {
const {data, isPending, isError} = fullLogDataResult;
const isTableFrozen = useLogsIsTableFrozen();
const organization = useOrganization();
const showFilterButtons = !isTableFrozen;
Copy link

Choose a reason for hiding this comment

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

[BestPractice]

The showFilterButtons variable name doesn't accurately reflect the logic. Since isTableFrozen indicates that filtering should be disabled, consider renaming this variable to be more descriptive:

Suggested change
const showFilterButtons = !isTableFrozen;
const canFilter = !isTableFrozen;

Then update the conditional to use canFilter instead of showFilterButtons for better code clarity.

⚡ Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.


const {onClick: betterCopyToClipboard} = useCopyToClipboard({
text: isPending || isError ? '' : ourlogToJson(data),
onCopy: () => {
trackAnalytics('logs.table.row_copied_as_json', {
log_id: String(tableDataRow[OurLogKnownFieldKey.ID]),
organization,
});
},

successMessage: t('Copied!'),
errorMessage: t('Failed to copy'),
});
return (
<Fragment>
{showFilterButtons ? (
<LogRowDetailsFilterActions tableDataRow={tableDataRow} />
) : (
<span />
Comment on lines +549 to +552
Copy link

Choose a reason for hiding this comment

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

[BestPractice]

Consider using a more semantic placeholder instead of an empty <span />. This could improve accessibility and make the intent clearer:

Suggested change
{showFilterButtons ? (
<LogRowDetailsFilterActions tableDataRow={tableDataRow} />
) : (
<span />
{showFilterButtons ? (
<LogRowDetailsFilterActions tableDataRow={tableDataRow} />
) : null}

⚡ Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

)}
<LogDetailTableActionsButtonBar>
<Button
priority="link"
size="sm"
borderless
onClick={() => {
betterCopyToClipboard();
}}
>
<IconJson size="md" style={{paddingRight: space(0.5)}} />
{t('Copy as JSON')}
</Button>
</LogDetailTableActionsButtonBar>
</Fragment>
);
}