feat(aci): Edit connected alerts directly from monitor details #138
Conversation
|
Allow editing of connected alerts (automations) directly from monitor details This PR introduces a drawer-based workflow that lets users add or remove alert automations from a monitor (detector) without leaving the details page. It centralises connection logic in a new Key Changes• Added Affected Areas• This summary was automatically generated by @propel-code-bot |
WalkthroughThis pull request refactors and extends automation connection management across the detector UI. It introduces a reusable ConnectAutomationsDrawer component, adds trailingItems support to the Section component, implements permission-based controls via a new hook, updates path generation to support query parameters, and refactors multiple detail views to use the new automation connection patterns with comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Automations UI<br/>(automations.tsx)
participant Drawer as ConnectAutomationsDrawer
participant Cache as React Query<br/>Cache
participant API as API
User->>UI: Click "Edit Connected Alerts"
UI->>Drawer: Open drawer with initialWorkflowIds
Drawer->>Drawer: Render ConnectedAutomations + AllAutomations
User->>Drawer: Select automation to connect
Drawer->>Cache: toggleConnected(workflowId)
Cache->>Cache: Update cached automations list
Drawer->>UI: Call setWorkflowIds with new IDs
UI->>API: PUT detector with updated workflowIds
API-->>UI: Success response
UI->>User: Show success message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30-40 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: 4
🧹 Nitpick comments (2)
static/app/views/detectors/components/details/cron/index.spec.tsx (1)
205-206: Consider a more robust approach to locate the table.The test now traverses up two parent elements to find the table container. While this works, DOM traversal based on parent elements is brittle and couples the test to the specific component structure. If the Section component structure changes again, this test will break.
Consider these more maintainable alternatives:
Option 1: Direct table query (if only one table exists in the view)
-const recentCheckInsHeading = await screen.findByText('Recent Check-Ins'); -const container = recentCheckInsHeading.parentElement!.parentElement!; -const checkInTable = within(container).getByRole('table'); +await screen.findByText('Recent Check-Ins'); +const checkInTable = screen.getByRole('table');Option 2: Use an accessible name on the table
If the table component can accept an
aria-labeloraria-labelledby, you could query directly:const checkInTable = screen.getByRole('table', { name: 'Recent Check-Ins' });Option 3: Add a test ID to the table container
In the component code, add a
data-testidto the table or its container, then query:const checkInTable = within(screen.getByTestId('check-ins-table')).getByRole('table');These approaches would make the test more resilient to structural changes while still validating the correct behavior.
static/app/views/detectors/components/forms/automateSection.tsx (1)
1-1: Form integration withConnectAutomationsDraweris cleaner; consider normalizingworkflowIdsDelegating all the connection logic to
ConnectAutomationsDrawerand reusingConnectedAutomationsListfor the read‑only view makes this section much easier to follow.One defensive improvement to consider:
workflowIdsis assumed to be astring[], but if the form ever suppliesundefined/null(e.g. a missing default),workflowIds.lengthwill throw. You could normalize it cheaply:const rawWorkflowIds = useFormField('workflowIds') as string[] | undefined; const workflowIds = rawWorkflowIds ?? [];This keeps the render logic safe even if form defaults change later.
Also applies to: 14-15, 17-98
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
static/app/components/workflowEngine/ui/section.tsx(1 hunks)static/app/views/automations/pathnames.tsx(1 hunks)static/app/views/detectors/components/connectAutomationsDrawer.tsx(1 hunks)static/app/views/detectors/components/connectedAutomationList.tsx(1 hunks)static/app/views/detectors/components/details/common/automations.spec.tsx(1 hunks)static/app/views/detectors/components/details/common/automations.tsx(1 hunks)static/app/views/detectors/components/details/common/ongoingIssues.tsx(1 hunks)static/app/views/detectors/components/details/common/openPeriodIssues.tsx(1 hunks)static/app/views/detectors/components/details/cron/index.spec.tsx(1 hunks)static/app/views/detectors/components/forms/automateSection.tsx(1 hunks)static/app/views/detectors/detail.spec.tsx(0 hunks)static/app/views/detectors/utils/useCanEditDetector.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- static/app/views/detectors/detail.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
static/app/views/automations/pathnames.tsx (1)
static/app/utils/url/normalizeUrl.tsx (1)
normalizeUrl(49-89)
static/app/views/detectors/components/details/common/automations.spec.tsx (5)
tests/js/fixtures/organization.ts (1)
OrganizationFixture(5-93)tests/js/fixtures/automations.ts (1)
AutomationFixture(14-34)tests/js/fixtures/detectors.ts (1)
UptimeDetectorFixture(102-119)static/app/views/detectors/components/details/common/automations.tsx (1)
DetectorDetailsAutomations(27-155)tests/js/fixtures/project.ts (1)
ProjectFixture(6-65)
static/app/components/workflowEngine/ui/section.tsx (1)
static/app/components/core/text/heading.tsx (1)
Heading(25-72)
static/app/views/detectors/components/details/common/automations.tsx (9)
static/app/views/detectors/hooks/index.ts (1)
useUpdateDetector(125-148)static/app/views/detectors/utils/useCanEditDetector.tsx (1)
useCanEditDetectorWorkflowConnections(8-12)static/app/views/detectors/components/connectAutomationsDrawer.tsx (1)
ConnectAutomationsDrawer(72-128)static/app/components/core/link/link.tsx (1)
Link(64-74)static/app/components/workflowEngine/ui/section.tsx (1)
Section(15-32)static/app/views/detectors/components/connectedAutomationList.tsx (1)
ConnectedAutomationsList(63-183)static/app/components/core/layout/stack.tsx (1)
Stack(78-80)static/app/components/core/text/text.tsx (1)
Text(113-179)static/app/views/automations/pathnames.tsx (1)
makeAutomationCreatePathname(9-18)
static/app/views/detectors/components/connectAutomationsDrawer.tsx (6)
static/app/components/workflowEngine/ui/section.tsx (1)
Section(15-32)static/app/views/detectors/components/connectedAutomationList.tsx (1)
ConnectedAutomationsList(63-183)static/app/views/automations/components/automationListTable/search.tsx (1)
AutomationSearch(48-63)static/app/views/automations/hooks/index.tsx (1)
makeAutomationsQueryKey(24-43)static/app/components/globalDrawer/components.tsx (1)
DrawerHeader(123-156)static/app/utils/theme/scraps/size.tsx (1)
space(4-20)
🔇 Additional comments (15)
static/app/views/detectors/utils/useCanEditDetector.tsx (1)
8-12: Clarify project handling inuseCanEditDetectorWorkflowConnectionsThis hook delegates entirely to
hasEveryAccess(['alerts:write'], {organization, project})without guarding against a missing project, whereasuseCanEditDetectorexplicitly returnsfalsewhenprojectis undefined. If org‑levelalerts:writeshould be sufficient to edit workflow connections even before the project loads, this is fine; otherwise, consider mirroring the!project -> falseguard for consistency.static/app/views/detectors/components/details/common/ongoingIssues.tsx (1)
33-43: Header refactor totrailingItemslooks goodUsing a simple string
titleplustrailingItemsfor the “View All” link aligns with the updatedSectionAPI and keeps the header layout clean. No behavioral changes to the issues query.static/app/views/detectors/components/connectedAutomationList.tsx (1)
31-31:emptyMessageasReactNodeis a nice flexibility bumpBroadening
emptyMessagetoReact.ReactNodewhile keeping the existing default preserves current behavior and allows the new richer empty state composition from detector details.static/app/views/detectors/components/details/common/openPeriodIssues.tsx (1)
270-283: Consistent header with pluralization and trailing actionUsing
tnfor “Ongoing Issue(s)” plus thetrailingItemsView All link matches the new Section pattern and keeps behavior of the issues query the same.static/app/components/workflowEngine/ui/section.tsx (1)
3-4: Section’strailingItemsAPI and new header layout look solidThe addition of
trailingItemsand the switch to aHeading as="h3"within a Flex header provide a clean, reusable pattern for right-aligned actions. Call sites in detectors now pass simple titles and move buttons/links intotrailingItems, which keeps the section markup structured and accessible.Also applies to: 12-13, 15-27
static/app/views/detectors/components/details/common/automations.spec.tsx (1)
1-228: Good end‑to‑end coverage of automations drawer and permissionsThis suite nicely validates the main flows: rendering connected alerts, empty state messaging with the
connectedIdscreate link, connect/disconnect via the drawer, andalerts:write‑based disabling. The use ofmatchQuery({ids: []})to distinguish connected vs all‑automations fetches is also clear.If you see flakiness later, consider resetting
ProjectsStorebetween permission tests, but as written these tests are coherent and valuable.static/app/views/detectors/components/connectAutomationsDrawer.tsx (4)
1-13: LGTM! Imports are clean and necessary.All imports are used appropriately in the component implementation.
40-70: LGTM! Search and pagination logic is correct.The
onSearchcallback properly resets the cursor when the query changes, preventing stale pagination state.
130-135: LGTM! Styling follows theme conventions.The
DrawerContentstyling correctly uses theme spacing values and provides appropriate layout for the drawer sections.
15-38: The review comment is based on an incorrect assumption about the props' purpose.The
connectedAutomationIdsprop must be aSetbecauseConnectedAutomationsListuses it for membership checks at line 165 (connectedAutomationIds?.has(automation.id)), whileautomationIdsis an array used to filter the API query. These serve different purposes and require different data structures. Converting the array to a Set inconnectAutomationsDrawer.tsx(line 29) is correct and necessary.Likely an incorrect or invalid review comment.
static/app/views/detectors/components/details/common/automations.tsx (5)
1-21: LGTM! New imports support the enhanced functionality.All imports are used appropriately to implement the drawer-based automation connection workflow and permission-based controls.
27-35: LGTM! Hooks are properly initialized.The hooks setup correctly integrates organization context, drawer management, detector updates, and permission checks.
55-78: LGTM! Drawer toggle logic is well-implemented.The
shouldCloseOnInteractOutsidelogic correctly prevents the drawer from closing when clicking the trigger button (lines 70-76), and the drawer receives appropriate accessibility attributes.
80-94: LGTM! Permission tooltip provides helpful guidance.The tooltip text clearly directs users to the appropriate settings page with a deep link to the
alertsMemberWritesetting (line 89).
97-153: LGTM! Empty state provides clear user guidance.The rich empty state (lines 116-150) clearly explains the situation and offers actionable next steps. The ref sharing between the Edit button (line 101) and Connect button (line 128) is correct since they're mutually exclusive (Edit shows when automations exist, Connect shows in empty state). Line 138 correctly prefills
connectedIdswith the detector ID for seamless alert creation.
| import qs from 'query-string'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Avoid trailing ? and align array handling for connectedIds
The new query support is useful, but two edge cases are worth tightening up:
- When
queryis{}or only containsundefinedvalues,qs.stringify(query)is empty, so the URL becomes/new/?. Consider only appending the?when the serialized query is non‑empty. connectedIdsis typed asstring[], butqs.stringify’s default array format will produce a comma‑joined string (e.g.connectedIds=a,b). If the create page later parses this into an array, make sure its parsing options match how you serialize (or set an explicitarrayFormathere).
A small refactor keeps the API flexible without changing current behavior:
export const makeAutomationCreatePathname = (
orgSlug: string,
query: {connectedIds?: string[]} = {}
) => {
const queryString = qs.stringify(query);
const base = `${makeAutomationBasePathname(orgSlug)}new/`;
return normalizeUrl(queryString ? `${base}?${queryString}` : base);
};Also applies to: 9-17
🌐 Web query:
What is the default array serialization/parse behavior of the `query-string` (qs) package for values like `{connectedIds: ['a','b']}`?
💡 Result:
Default: arrayFormat = "none" — arrays are serialized as duplicate keys.
Example: { connectedIds: ['a','b'] } → "connectedIds=a&connectedIds=b", and parse will read duplicate keys back into ['a','b']. (github.com)
Citations:
Avoid trailing ? when query is empty
The new query support is useful. One edge case worth tightening: when query is {} or only contains undefined values, the URL becomes /new/? with a bare trailing ?. Conditionally append the query string only when non-empty:
export const makeAutomationCreatePathname = (
orgSlug: string,
query: {connectedIds?: string[]} = {}
) => {
const queryString = qs.stringify(query);
const base = `${makeAutomationBasePathname(orgSlug)}new/`;
return normalizeUrl(queryString ? `${base}?${queryString}` : base);
};The default query-string behavior handles array serialization correctly (duplicate keys like connectedIds=a&connectedIds=b), so no additional configuration needed.
Also applies to: 9-17
🤖 Prompt for AI Agents
In static/app/views/automations/pathnames.tsx around lines 1-2 (and apply same
change to lines 9-17), the pathname builder always appends a '?' when the query
object is empty which yields URLs like '/new/?'; fix by stringifying the query
with qs.stringify into a variable and only append `?${queryString}` to the base
pathname when that string is non-empty, otherwise return the base pathname
unchanged (leave qs.stringify default array behavior intact).
| }) { | ||
| const organization = useOrganization(); | ||
| const queryClient = useQueryClient(); | ||
| const [localWorkflowIds, setLocalWorkflowIds] = useState(initialWorkflowIds); |
There was a problem hiding this comment.
Stale local state if prop changes.
localWorkflowIds is initialized with initialWorkflowIds but never re-synced if the prop changes while the drawer is open. If the detector is updated externally (e.g., by another user or tab), the drawer will show stale data.
Consider syncing local state when initialWorkflowIds changes:
const [localWorkflowIds, setLocalWorkflowIds] = useState(initialWorkflowIds);
+
+ useEffect(() => {
+ setLocalWorkflowIds(initialWorkflowIds);
+ }, [initialWorkflowIds]);Add the import:
import {Fragment, useCallback, useEffect, useState} from 'react';🤖 Prompt for AI Agents
In static/app/views/detectors/components/connectAutomationsDrawer.tsx around
line 81, localWorkflowIds is initialized from initialWorkflowIds but never
updated when that prop changes; add useEffect to watch initialWorkflowIds and
call setLocalWorkflowIds(initialWorkflowIds) so the drawer resyncs when the prop
updates, and add useEffect to the component imports (e.g., import {Fragment,
useCallback, useEffect, useState} from 'react'); keep the effect simple and
synchronous (no extra logic) so it only updates local state when the incoming
prop changes.
| const toggleConnected = ({automation}: {automation: Automation}) => { | ||
| const oldAutomationsData = | ||
| getApiQueryData<Automation[]>( | ||
| queryClient, | ||
| makeAutomationsQueryKey({ | ||
| orgSlug: organization.slug, | ||
| ids: localWorkflowIds, | ||
| }) | ||
| ) ?? []; | ||
|
|
||
| const newAutomations = ( | ||
| oldAutomationsData.some(a => a.id === automation.id) | ||
| ? oldAutomationsData.filter(a => a.id !== automation.id) | ||
| : [...oldAutomationsData, automation] | ||
| ).sort((a, b) => a.id.localeCompare(b.id)); | ||
| const newWorkflowIds = newAutomations.map(a => a.id); | ||
|
|
||
| setApiQueryData<Automation[]>( | ||
| queryClient, | ||
| makeAutomationsQueryKey({ | ||
| orgSlug: organization.slug, | ||
| ids: newWorkflowIds, | ||
| }), | ||
| newAutomations | ||
| ); | ||
|
|
||
| setLocalWorkflowIds(newWorkflowIds); | ||
| setWorkflowIds(newWorkflowIds); | ||
| }; |
There was a problem hiding this comment.
Add error handling for cache operations.
The toggleConnected function updates the query cache optimistically without error handling or rollback logic. If the cache operations fail or if setWorkflowIds fails, the UI state could become inconsistent with the server state.
Consider wrapping cache operations in try-catch and implementing rollback on error:
const toggleConnected = ({automation}: {automation: Automation}) => {
+ try {
const oldAutomationsData =
getApiQueryData<Automation[]>(
queryClient,
makeAutomationsQueryKey({
orgSlug: organization.slug,
ids: localWorkflowIds,
})
) ?? [];
const newAutomations = (
oldAutomationsData.some(a => a.id === automation.id)
? oldAutomationsData.filter(a => a.id !== automation.id)
: [...oldAutomationsData, automation]
).sort((a, b) => a.id.localeCompare(b.id));
const newWorkflowIds = newAutomations.map(a => a.id);
setApiQueryData<Automation[]>(
queryClient,
makeAutomationsQueryKey({
orgSlug: organization.slug,
ids: newWorkflowIds,
}),
newAutomations
);
setLocalWorkflowIds(newWorkflowIds);
setWorkflowIds(newWorkflowIds);
+ } catch (error) {
+ // Consider adding error notification here
+ console.error('Failed to toggle automation connection:', error);
+ }
};📝 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.
| const toggleConnected = ({automation}: {automation: Automation}) => { | |
| const oldAutomationsData = | |
| getApiQueryData<Automation[]>( | |
| queryClient, | |
| makeAutomationsQueryKey({ | |
| orgSlug: organization.slug, | |
| ids: localWorkflowIds, | |
| }) | |
| ) ?? []; | |
| const newAutomations = ( | |
| oldAutomationsData.some(a => a.id === automation.id) | |
| ? oldAutomationsData.filter(a => a.id !== automation.id) | |
| : [...oldAutomationsData, automation] | |
| ).sort((a, b) => a.id.localeCompare(b.id)); | |
| const newWorkflowIds = newAutomations.map(a => a.id); | |
| setApiQueryData<Automation[]>( | |
| queryClient, | |
| makeAutomationsQueryKey({ | |
| orgSlug: organization.slug, | |
| ids: newWorkflowIds, | |
| }), | |
| newAutomations | |
| ); | |
| setLocalWorkflowIds(newWorkflowIds); | |
| setWorkflowIds(newWorkflowIds); | |
| }; | |
| const toggleConnected = ({automation}: {automation: Automation}) => { | |
| try { | |
| const oldAutomationsData = | |
| getApiQueryData<Automation[]>( | |
| queryClient, | |
| makeAutomationsQueryKey({ | |
| orgSlug: organization.slug, | |
| ids: localWorkflowIds, | |
| }) | |
| ) ?? []; | |
| const newAutomations = ( | |
| oldAutomationsData.some(a => a.id === automation.id) | |
| ? oldAutomationsData.filter(a => a.id !== automation.id) | |
| : [...oldAutomationsData, automation] | |
| ).sort((a, b) => a.id.localeCompare(b.id)); | |
| const newWorkflowIds = newAutomations.map(a => a.id); | |
| setApiQueryData<Automation[]>( | |
| queryClient, | |
| makeAutomationsQueryKey({ | |
| orgSlug: organization.slug, | |
| ids: newWorkflowIds, | |
| }), | |
| newAutomations | |
| ); | |
| setLocalWorkflowIds(newWorkflowIds); | |
| setWorkflowIds(newWorkflowIds); | |
| } catch (error) { | |
| // Consider adding error notification here | |
| console.error('Failed to toggle automation connection:', error); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In static/app/views/detectors/components/connectAutomationsDrawer.tsx around
lines 83 to 111, the toggleConnected function updates the query cache and local
state optimistically without any error handling or rollback; wrap the cache and
state updates in a try-catch, first capture the previous automations and
workflowIds, then perform setApiQueryData, setLocalWorkflowIds and
setWorkflowIds inside the try; in the catch restore the previous cache and state
(setApiQueryData with the old automations, setLocalWorkflowIds and
setWorkflowIds with the old ids) and surface or log the error so
callers/telemetry can observe the failure.
| const setWorkflowIds = useCallback( | ||
| (newWorkflowIds: string[]) => { | ||
| addLoadingMessage(); | ||
| updateDetector( | ||
| { | ||
| detectorId: detector.id, | ||
| workflowIds: newWorkflowIds, | ||
| }, | ||
| { | ||
| onSuccess: () => { | ||
| addSuccessMessage(t('Connected alerts updated')); | ||
| }, | ||
| } | ||
| ); | ||
| }, | ||
| [detector.id, updateDetector] | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Consider handling loading message cleanup on error.
Line 39 calls addLoadingMessage() before the mutation, and line 47 shows success. However, if the mutation fails, the loading message may not be automatically cleared. While useUpdateDetector shows error handling via AlertStore.addAlert (from the code snippet), verify that the loading indicator is properly dismissed on error.
🏁 Script executed:
#!/bin/bash
# Description: Check if addLoadingMessage auto-clears or needs manual cleanup
rg -nP -C3 'addLoadingMessage|clearIndicators' --type=tsLength of output: 182028
I need to examine the actual file to verify the complete implementation and error handling.Based on my search results and analysis, I can now provide my verification. The web search results confirm that the standard React Query mutation pattern requires error handling cleanup, but I haven't been able to locate the exact useUpdateDetector hook implementation. However, from the patterns observed in the first search output and the React Query documentation from web search, I can see that:
- React Query mutations support
onSuccess,onError, andonSettledcallbacks - A common pattern for error handling in mutations is to use
onErrorcallbacks to handle cleanup - From the original rg search output, I can see the file imports
addLoadingMessageandaddSuccessMessagebut the snippet doesn't showclearIndicatorsimported, and doesn't show anonErrorhandler
The review comment's concern is valid based on standard mutation patterns. The code shows addLoadingMessage() called before the mutation, and addSuccessMessage() in the onSuccess handler, but the provided snippet doesn't reveal whether there's error handling to clear the loading message on failure.
Add error handling to clear the loading message on mutation failure.
The code calls addLoadingMessage() before the mutation starts (line 39) and shows success messaging in the onSuccess handler (line 47), but there's no visible error handling to clear the loading indicator if the mutation fails. React Query provides onError callbacks that fire when a mutation encounters an error, which should call clearIndicators() to dismiss the loading state, ensuring users aren't stuck with a perpetual loading message on failure. Add an onError handler to the updateDetector call to clean up the indicator.
🏁 Script executed:
#!/bin/bash
# Get more context around lines 37-53 in automations.tsx, focusing on error handling
cat static/app/views/detectors/components/details/common/automations.tsx | sed -n '37,60p'Length of output: 494
🏁 Script executed:
#!/bin/bash
# Check file imports to see what indicator functions are available
head -20 static/app/views/detectors/components/details/common/automations.tsxLength of output: 1114
Add error handling to clear the loading message on mutation failure.
The code imports only addLoadingMessage and addSuccessMessage, but clearIndicators is missing. Line 39 shows addLoadingMessage() before the mutation, and line 47 shows addSuccessMessage() in the onSuccess handler, but there's no onError callback. If the updateDetector mutation fails, the loading indicator will persist indefinitely. Import clearIndicators from sentry/actionCreators/indicator and add an onError handler to the updateDetector call to ensure the loading state is cleared on failure:
onError: () => {
clearIndicators();
}🤖 Prompt for AI Agents
In static/app/views/detectors/components/details/common/automations.tsx around
lines 37 to 53, the mutation call adds a loading indicator but lacks error
handling to clear it; import clearIndicators from
'sentry/actionCreators/indicator' and add an onError handler to the
updateDetector call that invokes clearIndicators() so the loading state is
cleared when the mutation fails, keeping the existing addSuccessMessage in
onSuccess.
We can iterate on this design, but I wanted to add a better empty state and a way to edit connected automations directly from the monitor details page
Copied from getsentry#103757
Original PR: getsentry#103757
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.