Skip to content

Conversation

@jasonyuezhang
Copy link
Owner

rather than adding a temporary type field on condition subfilters, this change adds logic to check for either an attribute or key field on the subfilter to determine if it's an attribute or tag filter

using the type to determine the filter (attribute/tag) would not work when loading an existing alert into the builder when editing an alert, since it was just a temporary field being added in the ui during the alert creation flow


Copied from getsentry#101972
Original PR: getsentry#101972

@propel-test-bot
Copy link

Eliminate temporary type field and infer sub-filter kind from attribute/key

Bug-fix PR that removes the ad-hoc type discriminator previously added only on the frontend for alert sub-filters. The UI now determines whether a sub-filter is an attribute or tag comparison by the presence of attribute or key fields, which allows existing alerts to load correctly in the builder. Serialization logic is updated to stop stripping a now-removed type field.

Key Changes

• Refactored ComparisonTypeField in static/app/views/automations/components/actionFilters/subfiltersList.tsx to render based on 'attribute' in subfilter || 'key' in subfilter instead of subfilter.type
• Removed creation and propagation of temporary type when building new sub-filters; onUpdate now sets either attribute or key directly
• Simplified payload sanitation in static/app/views/automations/components/automationFormData.tsx by replacing stripSubfilterTypeAndId with stripSubfilterId (only id is stripped)
• Adjusted select onChange logic to build sub-filter objects without a type property and with default attribute or key placeholders

Affected Areas

actionFilters/subfiltersList.tsx (UI rendering & state mutation)
automationFormData.tsx (form serialization before API submission)
• Any consumer code that formerly relied on subfilter.type

This summary was automatically generated by @propel-code-bot

Comment on lines +33 to 35
const stripSubfilterId = (subfilter: any) => {
const {id: _id, ...subfilterWithoutTypeAndId} = subfilter;
return subfilterWithoutTypeAndId;

Choose a reason for hiding this comment

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

[BestPractice]

The function stripSubfilterId has a misleading variable name. It's destructuring ...subfilterWithoutTypeAndId but the function no longer removes the type field - only the id field. This creates confusion about what the function actually does.

Suggested change
const stripSubfilterId = (subfilter: any) => {
const {id: _id, ...subfilterWithoutTypeAndId} = subfilter;
return subfilterWithoutTypeAndId;
const stripSubfilterId = (subfilter: any) => {
const {id: _id, ...subfilterWithoutId} = subfilter;
return subfilterWithoutId;
};

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.

Context for Agents
[**BestPractice**]

The function `stripSubfilterId` has a misleading variable name. It's destructuring `...subfilterWithoutTypeAndId` but the function no longer removes the `type` field - only the `id` field. This creates confusion about what the function actually does.

```suggestion
const stripSubfilterId = (subfilter: any) => {
  const {id: _id, ...subfilterWithoutId} = subfilter;
  return subfilterWithoutId;
};
```

⚡ **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.

File: static/app/views/automations/components/automationFormData.tsx
Line: 35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants