feat(aci): Add simpler UX for connecting alerts to a project #104221
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
ameliahsu
reviewed
Dec 2, 2025
| label={t('Connected monitors mode')} | ||
| value={monitorMode} | ||
| choices={[ | ||
| ['project', t('Alert on all issues in a project')], |
Contributor
There was a problem hiding this comment.
Should this be something like "Alert on all issues in selected project(s)" since it's multiselect?
Member
Author
There was a problem hiding this comment.
I kept this wording because it is more simple/direct and the input is obviously multi-select, but I can see it going either way
| const {data: issueStreamDetectors} = useIssueStreamDetectors(); | ||
| const initialMode = getInitialMonitorMode(issueStreamDetectors, connectedIds); | ||
|
|
||
| if (!initialMode) { |
Contributor
There was a problem hiding this comment.
Should we be checking if the issue stream detector query is loading here instead of checking initialMode, then if the query fails we use the specific mode?
Member
Author
There was a problem hiding this comment.
Yeah true, that's better - updated!
…ng-alerts-to-a-project
Comment on lines
+204
to
+213
| // Sync the derived selectedProjectIds to the form model so the field can read from it | ||
| useEffect(() => { | ||
| const selectedProjectIds = | ||
| issueStreamDetectorsQuery.data | ||
| ?.filter(detector => connectedIds.includes(detector.id)) | ||
| .map(d => d.projectId) ?? []; | ||
| if (form && selectedProjectIds.length > 0) { | ||
| form.setValue('projectIds', selectedProjectIds); | ||
| } | ||
| }, [connectedIds, form, issueStreamDetectorsQuery.data]); |
There was a problem hiding this comment.
Bug: useEffect fails to clear projectIds in form model when selectedProjectIds is empty, causing stale data submission.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
When a user clears all project selections in SentryProjectSelectorField, the useEffect hook's condition selectedProjectIds.length > 0 prevents form.setValue('projectIds', selectedProjectIds) from being called. This leaves the form model's projectIds field with stale, previously selected values, even though connectedIds is correctly updated to []. Consequently, submitting the form sends incorrect, outdated projectIds, leading to incorrect automation connections.
💡 Suggested Fix
Remove the selectedProjectIds.length > 0 condition from the useEffect hook, ensuring form.setValue('projectIds', selectedProjectIds) is always called when form exists, even if selectedProjectIds is an empty array.
🤖 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/components/editConnectedMonitors.tsx#L204-L213
Potential issue: When a user clears all project selections in
`SentryProjectSelectorField`, the `useEffect` hook's condition
`selectedProjectIds.length > 0` prevents `form.setValue('projectIds',
selectedProjectIds)` from being called. This leaves the form model's `projectIds` field
with stale, previously selected values, even though `connectedIds` is correctly updated
to `[]`. Consequently, submitting the form sends incorrect, outdated `projectIds`,
leading to incorrect automation connections.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5003126
ameliahsu
approved these changes
Dec 2, 2025
| return connectedIds.every(id => issueStreamDetectors.find(d => d.id === id)) | ||
| ? 'project' | ||
| : 'specific'; | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: Query error defaults to wrong mode with connected monitors
The getInitialMonitorMode function incorrectly returns 'project' when issueStreamDetectors is undefined due to a failed query, even when connectedIds exist. Per the PR discussion, when the query fails, it should fall back to 'specific' mode to preserve existing behavior and show the user's connected monitors. Currently, a user editing an automation with specific monitors connected would see the project selector instead of their monitors if the issue stream detectors API call fails, potentially leading to data loss on save.
Additional Locations (1)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a radio button to the "connect monitors" section. The default flow will be for users to select one or multiple projects that will alert on any issue. We need to fetch all the issue stream monitors in order to match them up with the projects from the dropdown.
The other option is to connect specific monitors, which works the same as it does today.