Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
239 changes: 239 additions & 0 deletions static/app/views/automations/hooks/utils.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -412,4 +412,243 @@ describe('findConflictingConditions', () => {
conflictReason: 'Delete duplicate triggers to continue.',
});
});

describe('Seer activity trigger incompatibilities', () => {
const seerTriggers: DataConditionGroup = {
id: 'triggers',
logicType: DataConditionGroupLogicType.ANY_SHORT_CIRCUIT,
conditions: [
{
id: '1',
type: DataConditionType.SEER_ACTIVITY_TRIGGER,
comparison: ['rca_completed'],
},
],
};

it('flags event attribute filters as incompatible with Seer activity triggers', () => {
const actionFilters = [
{
id: 'actionFilter1',
logicType: DataConditionGroupLogicType.ALL,
conditions: [
{id: '2', type: DataConditionType.EVENT_ATTRIBUTE, comparison: {}},
],
},
];

const result = findConflictingConditions(seerTriggers, actionFilters);
expect(result).toEqual({
conflictingConditionGroups: {
actionFilter1: new Set(['2']),
},
conflictReason:
'The conditions highlighted in red are not compatible with Seer activity triggers.',
});
});

it('flags tagged event, level, and release filters as incompatible', () => {
const actionFilters = [
{
id: 'actionFilter1',
logicType: DataConditionGroupLogicType.ALL,
conditions: [
{id: '2', type: DataConditionType.TAGGED_EVENT, comparison: {}},
{id: '3', type: DataConditionType.LEVEL, comparison: {}},
{id: '4', type: DataConditionType.LATEST_RELEASE, comparison: true},
{
id: '5',
type: DataConditionType.LATEST_ADOPTED_RELEASE,
comparison: {},
},
],
},
];

const result = findConflictingConditions(seerTriggers, actionFilters);
expect(result).toEqual({
conflictingConditionGroups: {
actionFilter1: new Set(['2', '3', '4', '5']),
},
conflictReason:
'The conditions highlighted in red are not compatible with Seer activity triggers.',
});
});

it('flags frequency (slow) conditions as incompatible', () => {
const actionFilters = [
{
id: 'actionFilter1',
logicType: DataConditionGroupLogicType.ALL,
conditions: [
{
id: '2',
type: DataConditionType.EVENT_FREQUENCY_COUNT,
comparison: {value: 10},
},
{
id: '3',
type: DataConditionType.PERCENT_SESSIONS_COUNT,
comparison: {value: 5},
},
],
},
];

const result = findConflictingConditions(seerTriggers, actionFilters);
expect(result).toEqual({
conflictingConditionGroups: {
actionFilter1: new Set(['2', '3']),
},
conflictReason:
'The conditions highlighted in red are not compatible with Seer activity triggers.',
});
});

it('allows compatible issue attribute filters with Seer activity triggers', () => {
const actionFilters = [
{
id: 'actionFilter1',
logicType: DataConditionGroupLogicType.ALL,
conditions: [
{
id: '2',
type: DataConditionType.AGE_COMPARISON,
comparison: {comparison_type: AgeComparison.OLDER, value: 10},
},
{
id: '3',
type: DataConditionType.ASSIGNED_TO,
comparison: {targetType: 'Unassigned'},
},
],
},
];

const result = findConflictingConditions(seerTriggers, actionFilters);
expect(result).toEqual({
conflictingConditionGroups: {},
conflictReason: null,
});
});

it('clears conflicts with ANY logic when at least one condition is compatible', () => {
const actionFilters = [
{
id: 'actionFilter1',
logicType: DataConditionGroupLogicType.ANY_SHORT_CIRCUIT,
conditions: [
{id: '2', type: DataConditionType.EVENT_ATTRIBUTE, comparison: {}},
{
id: '3',
type: DataConditionType.AGE_COMPARISON,
comparison: {comparison_type: AgeComparison.OLDER, value: 10},
},
],
},
];

const result = findConflictingConditions(seerTriggers, actionFilters);
expect(result).toEqual({
conflictingConditionGroups: {},
conflictReason: null,
});
});

it('flags conflicts with ANY logic when all conditions are incompatible', () => {
const actionFilters = [
{
id: 'actionFilter1',
logicType: DataConditionGroupLogicType.ANY_SHORT_CIRCUIT,
conditions: [
{id: '2', type: DataConditionType.EVENT_ATTRIBUTE, comparison: {}},
{id: '3', type: DataConditionType.TAGGED_EVENT, comparison: {}},
],
},
];

const result = findConflictingConditions(seerTriggers, actionFilters);
expect(result).toEqual({
conflictingConditionGroups: {
actionFilter1: new Set(['2', '3']),
},
conflictReason:
'The conditions highlighted in red are not compatible with Seer activity triggers.',
});
});

it('clears conflicts with legacy ANY logic when at least one condition is compatible', () => {
const actionFilters = [
{
id: 'actionFilter1',
logicType: DataConditionGroupLogicType.ANY,
conditions: [
{id: '2', type: DataConditionType.EVENT_ATTRIBUTE, comparison: {}},
{
id: '3',
type: DataConditionType.AGE_COMPARISON,
comparison: {comparison_type: AgeComparison.OLDER, value: 10},
},
],
},
];

const result = findConflictingConditions(seerTriggers, actionFilters);
expect(result).toEqual({
conflictingConditionGroups: {},
conflictReason: null,
});
});

it('flags conflicts with legacy ANY logic when all conditions are incompatible', () => {
const actionFilters = [
{
id: 'actionFilter1',
logicType: DataConditionGroupLogicType.ANY,
conditions: [
{id: '2', type: DataConditionType.EVENT_ATTRIBUTE, comparison: {}},
{id: '3', type: DataConditionType.TAGGED_EVENT, comparison: {}},
],
},
];

const result = findConflictingConditions(seerTriggers, actionFilters);
expect(result).toEqual({
conflictingConditionGroups: {
actionFilter1: new Set(['2', '3']),
},
conflictReason:
'The conditions highlighted in red are not compatible with Seer activity triggers.',
});
});

it('does not flag Seer incompatibilities when there is no Seer activity trigger', () => {
const triggers: DataConditionGroup = {
id: 'triggers',
logicType: DataConditionGroupLogicType.ANY_SHORT_CIRCUIT,
conditions: [
{
id: '1',
type: DataConditionType.FIRST_SEEN_EVENT,
comparison: {},
},
],
};
const actionFilters = [
{
id: 'actionFilter1',
logicType: DataConditionGroupLogicType.ALL,
conditions: [
{id: '2', type: DataConditionType.EVENT_ATTRIBUTE, comparison: {}},
],
},
];

const result = findConflictingConditions(triggers, actionFilters);
expect(result).toEqual({
conflictingConditionGroups: {},
conflictReason: null,
});
});
});
});
78 changes: 78 additions & 0 deletions static/app/views/automations/hooks/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,32 @@ export function findConflictingConditions(
};
}

// Check for action filters incompatible with Seer activity triggers
const hasSeerActivityTrigger = triggers.conditions.some(
c => c.type === DataConditionType.SEER_ACTIVITY_TRIGGER
);
if (hasSeerActivityTrigger) {
const seerConflictingActionFilters: Record<string, Set<string>> = {};
let hasSeerConflicts = false;

for (const actionFilter of actionFilters) {
const conflicts = findSeerActivityIncompatibleConditions(actionFilter);
if (conflicts.size > 0) {
hasSeerConflicts = true;
seerConflictingActionFilters[actionFilter.id] = conflicts;
}
}

if (hasSeerConflicts) {
return {
conflictingConditionGroups: seerConflictingActionFilters,
conflictReason: t(
'The conditions highlighted in red are not compatible with Seer activity triggers.'
),
};
}
}

return {
conflictingConditionGroups: {},
conflictReason: null,
Expand All @@ -158,6 +184,33 @@ const frequencyTypes = new Set<DataConditionType>([
DataConditionType.EVENT_UNIQUE_USER_FREQUENCY_PERCENT,
]);

/**
* Filters that require a GroupEvent to evaluate. Activity-based triggers
* (like Seer activity) pass an Activity object, so these filters always
* return false and will block the workflow if the logic type is ALL.
*/
const eventRequiredConditions = new Set<DataConditionType>([
DataConditionType.EVENT_ATTRIBUTE,
DataConditionType.TAGGED_EVENT,
DataConditionType.LEVEL,
DataConditionType.LATEST_RELEASE,
DataConditionType.LATEST_ADOPTED_RELEASE,
]);

/**
* Slow conditions that require Snuba queries. Activity-based triggers
* cannot be enqueued for delayed evaluation, so these conditions are
* silently skipped.
*/
const slowConditions = new Set<DataConditionType>([
DataConditionType.EVENT_FREQUENCY_COUNT,
DataConditionType.EVENT_FREQUENCY_PERCENT,
DataConditionType.EVENT_UNIQUE_USER_FREQUENCY_COUNT,
DataConditionType.EVENT_UNIQUE_USER_FREQUENCY_PERCENT,
DataConditionType.PERCENT_SESSIONS_COUNT,
DataConditionType.PERCENT_SESSIONS_PERCENT,
]);
Comment on lines +205 to +212

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 slowConditions set is missing two frequency-based conditions, causing the UI to not warn users about silently non-functional automation rules.
Severity: MEDIUM

Suggested Fix

Add DataConditionType.EVENT_UNIQUE_USER_FREQUENCY_WITH_CONDITIONS_COUNT and DataConditionType.EVENT_UNIQUE_USER_FREQUENCY_WITH_CONDITIONS_PERCENT to the slowConditions set in utils.tsx to ensure they are correctly flagged as incompatible with Seer activity triggers.

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/automations/hooks/utils.tsx#L205-L212

Potential issue: The `slowConditions` set is intended to identify automation conditions
that are incompatible with Seer activity triggers because they require Snuba queries.
However, it omits `EVENT_UNIQUE_USER_FREQUENCY_WITH_CONDITIONS_COUNT` and
`EVENT_UNIQUE_USER_FREQUENCY_WITH_CONDITIONS_PERCENT`. As a result, when a user
configures an automation rule with a Seer activity trigger and one of these conditions,
the UI will not display a warning. The backend will silently ignore the condition,
leading to a rule that does not function as expected, with no feedback provided to the
user.

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


function findFirstSeenEventConflictingConditions(
conditionGroup: DataConditionGroup
): Set<string> {
Expand Down Expand Up @@ -253,6 +306,31 @@ function findConflictingPriorityConditions(
return conflictingConditions;
}

function findSeerActivityIncompatibleConditions(
conditionGroup: DataConditionGroup
): Set<string> {
const allIncompatibleConditions = new Set([
...eventRequiredConditions,
...slowConditions,
]);
const anyLogicTypes = new Set([
DataConditionGroupLogicType.ANY_SHORT_CIRCUIT,
DataConditionGroupLogicType.ANY,
]);
const incompatibleConditions = conditionGroup.conditions.filter(c =>
allIncompatibleConditions.has(c.type)
);
// With ANY logic, if at least one condition is compatible, the filter can still pass
if (
anyLogicTypes.has(conditionGroup.logicType) &&
incompatibleConditions.length !== conditionGroup.conditions.length
) {
return new Set<string>();
Comment on lines +324 to +328

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 function findSeerActivityIncompatibleConditions incorrectly flags compatible conditions as incompatible when using NONE logic, showing a false positive warning.
Severity: MEDIUM

Suggested Fix

Update findSeerActivityIncompatibleConditions to correctly handle the DataConditionGroupLogicType.NONE case. When the logic is NONE, event-required conditions should not be considered incompatible, as their falsy evaluation is the desired behavior.

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/automations/hooks/utils.tsx#L324-L328

Potential issue: The `findSeerActivityIncompatibleConditions` function incorrectly flags
event-required conditions as incompatible when the filter logic is set to `NONE`. With
`NONE` logic, all conditions must evaluate to false for the filter to pass. Since
event-required conditions always evaluate to false when given a Seer activity object,
they are actually compatible and satisfy the `NONE` constraint. The current
implementation does not handle this case, resulting in a false positive warning in the
UI, preventing users from creating a valid automation rule.

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

}
Comment thread
cursor[bot] marked this conversation as resolved.

return new Set<string>(incompatibleConditions.map(c => c.id));

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.

NONE logic false Seer conflicts

Low Severity

findSeerActivityIncompatibleConditions marks event-based filters as incompatible for Seer triggers even when the action filter uses none logic. Those handlers evaluate to non-triggering on activity payloads, which is what none requires, so the filter can still pass and actions can run despite the red conflict state.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4c97a6a. Configure here.

}

function findDuplicateTriggerConditions(triggers: DataConditionGroup): Set<string> {
const conditionCounts: Record<string, string[]> = {};
const duplicates = new Set<string>();
Expand Down
Loading