Skip to content

feat(aci): Edit connected alerts directly from monitor details #138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jasonyuezhang wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(aci): Edit connected alerts directly from monitor details #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 14 additions & 8 deletions static/app/components/workflowEngine/ui/section.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,30 @@
import styled from '@emotion/styled';

import {Heading} from '@sentry/scraps/text';

import {Flex} from 'sentry/components/core/layout';

type SectionProps = {
title: React.ReactNode;
children?: React.ReactNode;
className?: string;
description?: React.ReactNode;
trailingItems?: React.ReactNode;
};

export default function Section({children, className, title, description}: SectionProps) {
export default function Section({
children,
className,
title,
description,
trailingItems,
}: SectionProps) {
return (
<SectionContainer direction="column" gap="md" className={className}>
<SectionHeading>{title}</SectionHeading>
<Flex justify="between" align="end" gap="md">
<Heading as="h3">{title}</Heading>
{trailingItems && <Flex gap="md">{trailingItems}</Flex>}
</Flex>
{description && <SectionDescription>{description}</SectionDescription>}
{children}
</SectionContainer>
Expand Down Expand Up @@ -42,10 +54,4 @@ const SectionContainer = styled(Flex)`
}
`;

const SectionHeading = styled('h4')`
font-size: ${p => p.theme.fontSize.lg};
font-weight: ${p => p.theme.fontWeight.bold};
margin: 0;
`;

// moved above to reference in SectionContainer
13 changes: 11 additions & 2 deletions static/app/views/automations/pathnames.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,20 @@
import qs from 'query-string';

Comment on lines +1 to +2
Copy link

@coderabbitai coderabbitai bot Nov 20, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 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 query is {} or only contains undefined values, qs.stringify(query) is empty, so the URL becomes /new/?. Consider only appending the ? when the serialized query is non‑empty.
  • connectedIds is typed as string[], but qs.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 explicit arrayFormat here).

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).

import normalizeUrl from 'sentry/utils/url/normalizeUrl';

export const makeAutomationBasePathname = (orgSlug: string) => {
return normalizeUrl(`/organizations/${orgSlug}/monitors/alerts/`);
};

export const makeAutomationCreatePathname = (orgSlug: string) => {
return normalizeUrl(`${makeAutomationBasePathname(orgSlug)}new/`);
export const makeAutomationCreatePathname = (
orgSlug: string,
query: {
connectedIds?: string[];
} = {}
) => {
return normalizeUrl(
`${makeAutomationBasePathname(orgSlug)}new/?${qs.stringify(query)}`
);
};

export const makeAutomationDetailsPathname = (orgSlug: string, automationId: string) => {
Expand Down
135 changes: 135 additions & 0 deletions static/app/views/detectors/components/connectAutomationsDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
import {Fragment, useCallback, useState} from 'react';
import styled from '@emotion/styled';

import {DrawerHeader} from 'sentry/components/globalDrawer/components';
import Section from 'sentry/components/workflowEngine/ui/section';
import {t} from 'sentry/locale';
import {space} from 'sentry/styles/space';
import type {Automation} from 'sentry/types/workflowEngine/automations';
import {getApiQueryData, setApiQueryData, useQueryClient} from 'sentry/utils/queryClient';
import useOrganization from 'sentry/utils/useOrganization';
import {AutomationSearch} from 'sentry/views/automations/components/automationListTable/search';
import {makeAutomationsQueryKey} from 'sentry/views/automations/hooks';
import {ConnectedAutomationsList} from 'sentry/views/detectors/components/connectedAutomationList';

function ConnectedAutomations({
automationIds,
toggleConnected,
}: {
automationIds: string[];
toggleConnected: (params: {automation: Automation}) => void;
}) {
const [cursor, setCursor] = useState<string | undefined>(undefined);

return (
<Section title={t('Connected Alerts')}>
<ConnectedAutomationsList
data-test-id="drawer-connected-automations-list"
automationIds={automationIds}
connectedAutomationIds={new Set(automationIds)}
toggleConnected={toggleConnected}
cursor={cursor}
onCursor={setCursor}
limit={null}
openInNewTab
/>
</Section>
);
}

function AllAutomations({
automationIds,
toggleConnected,
}: {
automationIds: string[];
toggleConnected: (params: {automation: Automation}) => void;
}) {
const [searchQuery, setSearchQuery] = useState('');
const [cursor, setCursor] = useState<string | undefined>(undefined);
const onSearch = useCallback((query: string) => {
setSearchQuery(query);
setCursor(undefined);
}, []);

return (
<Section title={t('All Alerts')}>
<AutomationSearch initialQuery={searchQuery} onSearch={onSearch} />
<ConnectedAutomationsList
data-test-id="drawer-all-automations-list"
automationIds={null}
connectedAutomationIds={new Set(automationIds)}
toggleConnected={toggleConnected}
emptyMessage={t('No alerts found')}
cursor={cursor}
onCursor={setCursor}
query={searchQuery}
openInNewTab
/>
</Section>
);
}

export function ConnectAutomationsDrawer({
initialWorkflowIds,
setWorkflowIds,
}: {
initialWorkflowIds: string[];
setWorkflowIds: (workflowIds: string[]) => void;
}) {
const organization = useOrganization();
const queryClient = useQueryClient();
const [localWorkflowIds, setLocalWorkflowIds] = useState(initialWorkflowIds);
Copy link

@coderabbitai coderabbitai bot Nov 20, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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);
};
Comment on lines +83 to +111
Copy link

@coderabbitai coderabbitai bot Nov 20, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.


return (
<Fragment>
<DrawerHeader hideBar />
<DrawerContent>
<ConnectedAutomations
automationIds={localWorkflowIds}
toggleConnected={toggleConnected}
/>
<AllAutomations
automationIds={localWorkflowIds}
toggleConnected={toggleConnected}
/>
</DrawerContent>
</Fragment>
);
}

const DrawerContent = styled('div')`
display: flex;
flex-direction: column;
gap: ${space(2)};
padding: ${space(2)} ${space(3)};
`;
2 changes: 1 addition & 1 deletion static/app/views/detectors/components/connectedAutomationList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ type Props = React.HTMLAttributes<HTMLDivElement> & {
cursor: string | undefined;
onCursor: CursorHandler;
connectedAutomationIds?: Set<string>;
emptyMessage?: string;
emptyMessage?: React.ReactNode;
limit?: number | null;
openInNewTab?: boolean;
query?: string;
Expand Down
Loading