feat(workflow_engine): Add list of tags to help users define alerts #137
Conversation
|
Replace free-text tag input with dropdown populated from organization tags The PR converts the Key Changes• Replaced Affected Areas• This summary was automatically generated by @propel-code-bot |
WalkthroughThe taggedEvent automation component has been refactored to replace a text input field with a creatable select dropdown that dynamically loads organization tags. The component now uses hooks to fetch tag data asynchronously, displays a loading indicator during data retrieval, and manages selection through structured option values. Changes
Sequence DiagramsequenceDiagram
participant Component as taggedEvent
participant useOrg as useOrganization
participant useFetch as useFetchOrganizationTags
participant Select as Creatable Select
participant Update as condition.key
Component->>useOrg: Get organization
Component->>useFetch: Fetch tags
useFetch-->>Component: Loading...
Note over Component: Show loading indicator
useFetch-->>Component: Tag data loaded
Component->>Component: Build options (sorted + current key)
Component->>Select: Render with options
Select->>Select: User selects tag
Select->>Component: onChange(selectedValue)
Component->>Update: Update condition.key
Component->>Component: Clear errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
try trigger propel code review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
static/app/views/automations/components/actionFilters/taggedEvent.tsx (2)
58-64: Consider type consistency when prepending current key.The logic correctly preserves existing selections, but line 63 mixes object types—
tagscontains tag objects from the API, whilecondition.comparisonis a comparison object with{key, match, value}. Although both have a.keyproperty (which is all that's needed for mapping at lines 73-76), this creates semantic inconsistency.For better type safety, consider normalizing to a consistent shape:
if ( condition.comparison.key && !tags.some(tag => tag.key === condition.comparison.key) ) { - tags.unshift(condition.comparison); + tags.unshift({key: condition.comparison.key}); }
48-48: Optional: Remove redundant parameter.The
enabled: trueparameter is typically the default for data-fetching hooks and can be omitted for cleaner code.dataset: Dataset.ERRORS, useCache: true, - enabled: true, keepPreviousData: true,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
static/app/views/automations/components/actionFilters/taggedEvent.tsx(2 hunks)
🔇 Additional comments (2)
static/app/views/automations/components/actionFilters/taggedEvent.tsx (2)
1-9: LGTM! Imports align with PR objectives.The new imports support the tag fetching functionality and correctly scope to error events using
Dataset.ERRORS, which aligns with the PR goal of showing tags from error events only.
40-56: The review comment's concerns are not valid. The code is correct as written.
projectIds: ['-1'] is the correct API syntax. Evidence shows
discover/results.tsx:284explicitly comparescurrentQuery.project[0] === '-1'(string format), confirming the API accepts string'-1'as the sentinel value for querying all projects.LoadingIndicator in tct() renders correctly. The
tct()function is designed to interpolate React components into translated strings. Returning<LoadingIndicator />fromKeyFieldduring the loading state is the standard pattern and will display properly without breaking the layout.Likely an incorrect or invalid review comment.
| onChange={(e: SelectValue<MatchType>) => { | ||
| onUpdate({comparison: {...condition.comparison, key: e.value}}); |
There was a problem hiding this comment.
Fix incorrect type annotation.
The onChange handler is annotated as SelectValue<MatchType>, but MatchType is for the match field (line 93), not the key field. The key is a string representing a tag name.
Apply this diff to fix the type:
- onChange={(e: SelectValue<MatchType>) => {
+ onChange={(e: SelectValue<string>) => {
onUpdate({comparison: {...condition.comparison, key: e.value}});
removeError(condition.id);
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onChange={(e: SelectValue<MatchType>) => { | |
| onUpdate({comparison: {...condition.comparison, key: e.value}}); | |
| onChange={(e: SelectValue<string>) => { | |
| onUpdate({comparison: {...condition.comparison, key: e.value}}); |
🤖 Prompt for AI Agents
In static/app/views/automations/components/actionFilters/taggedEvent.tsx around
lines 77-78, the onChange handler is incorrectly typed as SelectValue<MatchType>
even though it updates the key (a string tag name); change the handler's
parameter type to SelectValue<string> (or the appropriate string alias) and keep
using e.value when calling onUpdate so the comparison.key is correctly typed as
a string.
![propel-test-bot[bot]](https://avatars.githubusercontent.com/u/175999184?s=60&v=4){kind=small}
| onChange={(e: SelectValue<MatchType>) => { | ||
| onUpdate({comparison: {...condition.comparison, key: e.value}}); | ||
| removeError(condition.id); | ||
| }} |
There was a problem hiding this comment.
[DataTypeCheck]
Type safety issue: The onChange handler expects SelectValue<MatchType>, but the actual data is tag keys (strings), not MatchType. This type mismatch could cause runtime issues or type errors.
Fix: Use the correct generic type:
| onChange={(e: SelectValue<MatchType>) => { | |
| onUpdate({comparison: {...condition.comparison, key: e.value}}); | |
| removeError(condition.id); | |
| }} | |
| onChange={(e: SelectValue<string>) => { | |
| onUpdate({comparison: {...condition.comparison, key: e.value}}); | |
| removeError(condition.id); | |
| }} |
⚡ 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
**Type safety issue**: The `onChange` handler expects `SelectValue<MatchType>`, but the actual data is tag keys (strings), not `MatchType`. This type mismatch could cause runtime issues or type errors.
**Fix**: Use the correct generic type:
```suggestion
onChange={(e: SelectValue<string>) => {
onUpdate({comparison: {...condition.comparison, key: e.value}});
removeError(condition.id);
}}
```
⚡ **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/actionFilters/taggedEvent.tsx
Line: 80| return <LoadingIndicator mini size={24} style={{alignItems: 'center'}} />; | ||
| } | ||
|
|
||
| const tags = tagOptions.sort((a, b) => a.key.localeCompare(b.key)); |
There was a problem hiding this comment.
[BestPractice]
Array mutation issue: The sort() method mutates the original tagOptions array in-place. If this array is shared (e.g., from a cached query result), this could cause unexpected side effects or affect other components.
Fix: Create a shallow copy before sorting:
| const tags = tagOptions.sort((a, b) => a.key.localeCompare(b.key)); | |
| const tags = [...tagOptions].sort((a, b) => a.key.localeCompare(b.key)); |
⚡ 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
**Array mutation issue**: The `sort()` method mutates the original `tagOptions` array in-place. If this array is shared (e.g., from a cached query result), this could cause unexpected side effects or affect other components.
**Fix**: Create a shallow copy before sorting:
```suggestion
const tags = [...tagOptions].sort((a, b) => a.key.localeCompare(b.key));
```
⚡ **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/actionFilters/taggedEvent.tsx
Line: 58| const {data: tagOptions, isLoading} = useFetchOrganizationTags( | ||
| { | ||
| orgSlug: organization.slug, | ||
| projectIds: ['-1'], | ||
| dataset: Dataset.ERRORS, | ||
| useCache: true, | ||
| enabled: true, | ||
| keepPreviousData: true, | ||
| }, | ||
| {} | ||
| ); | ||
|
|
||
| if (!tagOptions || isLoading) { | ||
| return <LoadingIndicator mini size={24} style={{alignItems: 'center'}} />; | ||
| } |
There was a problem hiding this comment.
[BestPractice]
Missing error handling: When useFetchOrganizationTags fails (network error, API failure, etc.), the component only shows a loading indicator indefinitely. Users have no feedback that something went wrong or how to recover.
Consider adding error state handling:
- Check for error state from the hook (e.g.,
const {data: tagOptions, isLoading, error} = useFetchOrganizationTags(...)) - Display an error message or fallback UI when fetch fails
- Potentially fall back to a plain text input if tags cannot be loaded
Context for Agents
**Missing error handling**: When `useFetchOrganizationTags` fails (network error, API failure, etc.), the component only shows a loading indicator indefinitely. Users have no feedback that something went wrong or how to recover.
**Consider adding error state handling**:
- Check for error state from the hook (e.g., `const {data: tagOptions, isLoading, error} = useFetchOrganizationTags(...)`)
- Display an error message or fallback UI when fetch fails
- Potentially fall back to a plain text input if tags cannot be loaded
File: static/app/views/automations/components/actionFilters/taggedEvent.tsx
Line: 56| options={Object.values(tags).map(tag => ({ | ||
| value: tag.key, | ||
| label: tag.key, | ||
| }))} |
There was a problem hiding this comment.
[BestPractice]
Unnecessary Object.values() call: The tags variable is already an array (from sort()), so wrapping it in Object.values() is redundant and could cause confusion.
Fix: Remove the unnecessary wrapper:
| options={Object.values(tags).map(tag => ({ | |
| value: tag.key, | |
| label: tag.key, | |
| }))} | |
| options={tags.map(tag => ({ | |
| value: tag.key, | |
| label: tag.key, | |
| }))} |
⚡ 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
**Unnecessary `Object.values()` call**: The `tags` variable is already an array (from `sort()`), so wrapping it in `Object.values()` is redundant and could cause confusion.
**Fix**: Remove the unnecessary wrapper:
```suggestion
options={tags.map(tag => ({
value: tag.key,
label: tag.key,
}))}
```
⚡ **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/actionFilters/taggedEvent.tsx
Line: 76|
try trigger propel code review again |
|
try trigger propel code review~ |
|
try trigger propel code review |
Description
Currently we use a free form text field for the
tagged eventcondition. We've had a few user issues that that the tagged event was configured on an event that hasn't been seen.This is only using the tags seen on error events; this is a bit closer to the intent of the condition and hope this clarifies the condition for the user.
Copied from getsentry#103711
Original PR: getsentry#103711
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.