Skip to content

feat(explore): Validate explore column editor fields #118377

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
nsdeschenes wants to merge 6 commits into
base: master
Choose a base branch
from
+153 −6
Open

feat(explore): Validate explore column editor fields #118377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6fd43b7
Validate explore column editor fields
nsdeschenes Jun 24, 2026
7bc6137
Cover typed explore column selections
nsdeschenes Jun 24, 2026
3fe0e2f
Sync changed validated explore fields
nsdeschenes Jun 25, 2026
0bb534e
Remove delete calls, since all these attributes are valid
nsdeschenes Jun 25, 2026
dc3dafe
Remove test file
nsdeschenes Jun 25, 2026
c3ad65e
Wait for current explore field validation
nsdeschenes Jun 25, 2026
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
3 changes: 2 additions & 1 deletion static/app/views/explore/spans/hooks/useValidateSpansTab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export function useValidateSpansTab({enabled = true}: UseValidateSpansTabArgs =
const groupBys = useQueryParamsGroupBys();
const visualizes = useQueryParamsVisualizes();

const {data, isLoading} = useQuery({
const {data, isFetching, isLoading} = useQuery({
...validateEventParamsOptions({
organization,
selection,
Expand All @@ -49,6 +49,7 @@ export function useValidateSpansTab({enabled = true}: UseValidateSpansTabArgs =

return {
data,
isFetching,
isLoading,
};
}
46 changes: 46 additions & 0 deletions static/app/views/explore/tables/columnEditorModal.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,24 @@ const booleanTags: TagCollection = {
},
};

const enrichedNumberTags: TagCollection = {
...numberTags,
'custom.duration': {
key: 'custom.duration',
name: 'custom.duration',
kind: FieldKind.MEASUREMENT,
},
};

const enrichedBooleanTags: TagCollection = {
...booleanTags,
'custom.enabled': {
key: 'custom.enabled',
name: 'custom.enabled',
kind: FieldKind.BOOLEAN,
},
};

describe('ColumnEditorModal', () => {
it('allows closes modal on apply', async () => {
const onClose = jest.fn();
Expand Down Expand Up @@ -371,4 +389,32 @@ describe('ColumnEditorModal', () => {
expect(columns[1]).toHaveTextContent('id');
expect(columns[1]).toHaveTextContent('string');
});

it('renders existing columns with types from supplied tags', async () => {
renderGlobalModal();

act(() => {
openModal(
modalProps => (
<ColumnEditorModal
{...modalProps}
columns={['custom.duration', 'custom.enabled']}
onColumnsChange={() => {}}
stringTags={stringTags}
numberTags={enrichedNumberTags}
booleanTags={enrichedBooleanTags}
/>
),
{onClose: jest.fn()}
);
});

expect(await screen.findByRole('button', {name: 'Apply'})).toBeInTheDocument();

const columns = screen.getAllByTestId('editor-column');
expect(columns[0]).toHaveTextContent('custom.duration');
expect(columns[0]).toHaveTextContent('number');
expect(columns[1]).toHaveTextContent('custom.enabled');
expect(columns[1]).toHaveTextContent('boolean');
});
});
110 changes: 105 additions & 5 deletions static/app/views/explore/tables/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {Fragment, useEffect} from 'react';
import {Fragment, useEffect, useMemo} from 'react';

import {FeatureBadge} from '@sentry/scraps/badge';
import {Button} from '@sentry/scraps/button';
Expand All @@ -9,8 +9,11 @@ import {Tooltip} from '@sentry/scraps/tooltip';

import {IconEdit} from 'sentry/icons/iconEdit';
import {t} from 'sentry/locale';
import type {TagCollection} from 'sentry/types/group';
import type {Confidence} from 'sentry/types/organization';
import {FieldKind} from 'sentry/utils/fields';
import {AttributeBreakdownsContent} from 'sentry/views/explore/components/attributeBreakdowns/content';
import {prettifyAttributeName} from 'sentry/views/explore/components/traceItemAttributes/utils';
import {Mode} from 'sentry/views/explore/contexts/pageParamsContext/mode';
import type {AggregatesTableResult} from 'sentry/views/explore/hooks/useExploreAggregatesTable';
import type {SpansTableResult} from 'sentry/views/explore/hooks/useExploreSpansTable';
Expand All @@ -24,11 +27,13 @@ import {
useSetQueryParamsAggregateFields,
useSetQueryParamsFields,
} from 'sentry/views/explore/queryParams/context';
import {useValidateSpansTab} from 'sentry/views/explore/spans/hooks/useValidateSpansTab';
import {AggregateColumnEditorModal} from 'sentry/views/explore/tables/aggregateColumnEditorModal';
import {AggregatesTable} from 'sentry/views/explore/tables/aggregatesTable';
import {ColumnEditorModal} from 'sentry/views/explore/tables/columnEditorModal';
import {SpansTable} from 'sentry/views/explore/tables/spansTable';
import {TracesTable} from 'sentry/views/explore/tables/tracesTable/index';
import type {EventValidationData} from 'sentry/views/explore/utils/validateEventParamsOptions';

interface BaseExploreTablesProps {
confidences: Confidence[];
Expand Down Expand Up @@ -58,17 +63,51 @@ export function ExploreTables(props: ExploreTablesProps) {
const {attributes: numberTags} = useSpanItemAttributes({}, 'number');
const {attributes: stringTags} = useSpanItemAttributes({}, 'string');
const {attributes: booleanTags} = useSpanItemAttributes({}, 'boolean');
const {data: validatedColumnsData, isFetching: isValidatingColumns} =
useValidateSpansTab({
enabled: tab === Tab.SPAN,
});
const {
validatedBooleanTags,
validatedFields,
validatedNumberTags,
validatedStringTags,
} = useMemo(
() =>
getValidatedColumnEditorData({
booleanTags,
fields,
numberTags,
stringTags,
validatedColumnsData,
}),
[booleanTags, fields, numberTags, stringTags, validatedColumnsData]
);

useEffect(() => {
if (tab !== Tab.SPAN || isValidatingColumns) {
return;
}

const fieldsChanged =
validatedFields.length !== fields.length ||
validatedFields.some((field, index) => field !== fields[index]);

if (fieldsChanged) {
setFields([...validatedFields]);
}
}, [fields, isValidatingColumns, setFields, tab, validatedFields]);

const openColumnEditor = () => {
openModal(
modalProps => (
<ColumnEditorModal
{...modalProps}
columns={fields}
columns={validatedFields}
onColumnsChange={setFields}
stringTags={stringTags}
numberTags={numberTags}
booleanTags={booleanTags}
stringTags={validatedStringTags}
numberTags={validatedNumberTags}
booleanTags={validatedBooleanTags}
/>
),
{closeEvents: 'escape-key'}
Comment on lines 103 to 113

@sentry sentry Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: The 'Edit Table' button is not disabled during column validation, allowing users to add invalid columns that are then silently removed after validation completes.
Severity: MEDIUM

Suggested Fix

Disable the 'Edit Table' button while isValidatingColumns is true. This prevents the user from opening the editor with potentially stale validation data and adding columns that will be removed. Alternatively, pre-validate columns within the modal before allowing the user to apply changes.

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/views/explore/tables/index.tsx#L101-L113

Potential issue: A race condition exists where the 'Edit Table' button is not disabled
while column validation is in progress. If a user opens the column editor and adds an
invalid column before the `useValidateSpansTab` hook completes its validation fetch, the
column is temporarily added. Once validation completes, a `useEffect` hook silently
removes the invalid column. This results in a confusing user experience where a
user-applied change is reverted without any notification or explanation.

Did we get this right? 👍 / 👎 to inform future reviews.

Expand Down Expand Up @@ -161,3 +200,64 @@ export function ExploreTables(props: ExploreTablesProps) {
</Fragment>
);
}

function getValidatedColumnEditorData({
booleanTags,
fields,
numberTags,
stringTags,
validatedColumnsData,
}: {
booleanTags: TagCollection;
fields: readonly string[];
numberTags: TagCollection;
stringTags: TagCollection;
validatedColumnsData?: EventValidationData;
}) {
const validatedBooleanTags = {...booleanTags};
const validatedNumberTags = {...numberTags};
const validatedStringTags = {...stringTags};
const invalidFields = new Set<string>();

for (const item of validatedColumnsData?.field ?? []) {
if (!item.name) {
continue;
}

if (!item.valid) {
invalidFields.add(item.name);
continue;
}

if (item.attrType === 'boolean') {
validatedBooleanTags[item.name] ??= {
key: item.name,
name: prettifyAttributeName(item.name),
kind: FieldKind.BOOLEAN,
};
}

if (item.attrType === 'number') {
validatedNumberTags[item.name] ??= {
key: item.name,
name: prettifyAttributeName(item.name),
kind: FieldKind.MEASUREMENT,
};
}

if (item.attrType === 'string') {
validatedStringTags[item.name] ??= {
key: item.name,
name: prettifyAttributeName(item.name),
kind: FieldKind.TAG,
};
}
}

return {
validatedBooleanTags,
validatedFields: fields.filter(field => !invalidFields.has(field)),
validatedNumberTags,
validatedStringTags,
};
}
Loading