Skip to content

feat(aci): redirect alerts nav to monitors #103325

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

Merged
merged 2 commits into from
Nov 15, 2025
+54 −2
Merged

feat(aci): redirect alerts nav to monitors #103325

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d656da7
feat(aci): redirect alerts nav to monitors
ameliahsu Nov 13, 2025
7672fcd
fix activeTo
ameliahsu Nov 13, 2025
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
4 changes: 4 additions & 0 deletions static/app/views/detectors/list/allMonitors.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import SentryDocumentTitle from 'sentry/components/sentryDocumentTitle';
import WorkflowEngineListLayout from 'sentry/components/workflowEngine/layout/list';
import {t} from 'sentry/locale';
import {AlertsRedirectNotice} from 'sentry/views/detectors/list/common/alertsRedirectNotice';
import {DetectorListActions} from 'sentry/views/detectors/list/common/detectorListActions';
import {DetectorListContent} from 'sentry/views/detectors/list/common/detectorListContent';
import {DetectorListHeader} from 'sentry/views/detectors/list/common/detectorListHeader';
Expand All @@ -23,6 +24,9 @@ export default function AllMonitors() {
description={DESCRIPTION}
docsUrl={DOCS_URL}
>
<AlertsRedirectNotice>
{t('Alert Rules have been moved to Monitors and Alerts.')}
</AlertsRedirectNotice>
<DetectorListHeader />
<DetectorListContent {...detectorListQuery} />
</WorkflowEngineListLayout>
Expand Down
37 changes: 37 additions & 0 deletions static/app/views/detectors/list/common/alertsRedirectNotice.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import {parseAsBoolean, useQueryState} from 'nuqs';

import {Alert} from '@sentry/scraps/alert';
import {Button} from '@sentry/scraps/button';

import {IconClose} from 'sentry/icons';
import {t} from 'sentry/locale';

export function AlertsRedirectNotice({children}: {children: React.ReactNode}) {
const [wasRedirectedFromAlerts, setWasRedirectedFromAlerts] = useQueryState(
'alertsRedirect',
parseAsBoolean.withOptions({history: 'replace'}).withDefault(false)
);

if (!wasRedirectedFromAlerts) {
return null;
}

return (
<Alert
type="info"
trailingItems={
<Button
size="zero"
borderless
icon={<IconClose />}
aria-label={t('Dismiss')}
onClick={() => {
setWasRedirectedFromAlerts(false);
}}
/>
}
>
{children}
</Alert>
);
}
15 changes: 13 additions & 2 deletions static/app/views/nav/secondary/sections/issues/issuesSecondaryNav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import styled from '@emotion/styled';

import {t} from 'sentry/locale';
import useOrganization from 'sentry/utils/useOrganization';
import {useUser} from 'sentry/utils/useUser';
import {makeMonitorBasePathname} from 'sentry/views/detectors/pathnames';
import {ISSUE_TAXONOMY_CONFIG} from 'sentry/views/issueList/taxonomies';
import {useNavContext} from 'sentry/views/nav/context';
import {PRIMARY_NAV_GROUP_CONFIG} from 'sentry/views/nav/primary/config';
Expand Down Expand Up @@ -81,9 +83,18 @@ export function IssuesSecondaryNav() {
}

function ConfigureSection({baseUrl}: {baseUrl: string}) {
const user = useUser();
const organization = useOrganization();
const {layout} = useNavContext();
const isSticky = layout === NavLayout.SIDEBAR;

const shouldRedirectToWorkflowEngineUI =
!user.isStaff && organization.features.includes('workflow-engine-ui');
Comment on lines +91 to +92
Copy link

@sentry sentry bot Nov 13, 2025

Choose a reason for hiding this comment

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

Bug: useUser() can return null/undefined, but user.isStaff is accessed without a null check, causing a TypeError.
Severity: CRITICAL | Confidence: 0.95

🔍 Detailed Analysis

The useUser() hook can return null or undefined despite its Readonly<User> type annotation, as evidenced by explicit type casting and defensive checks in other parts of the codebase. The issuesSecondaryNav.tsx component directly accesses user.isStaff without a null/undefined check, leading to a TypeError: Cannot read property 'isStaff' of null/undefined if user is not yet initialized or becomes null/undefined during early app lifecycle or race conditions.

💡 Suggested Fix

Add a null/undefined check for the user object before accessing user.isStaff. For example, if (user && !user.isStaff && organization.features.includes('workflow-engine-ui')).

🤖 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/nav/secondary/sections/issues/issuesSecondaryNav.tsx#L91-L92

Potential issue: The `useUser()` hook can return `null` or `undefined` despite its
`Readonly<User>` type annotation, as evidenced by explicit type casting and defensive
checks in other parts of the codebase. The `issuesSecondaryNav.tsx` component directly
accesses `user.isStaff` without a null/undefined check, leading to a `TypeError: Cannot
read property 'isStaff' of null/undefined` if `user` is not yet initialized or becomes
`null`/`undefined` during early app lifecycle or race conditions.

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

Reference_id: 2663962


const alertsLink = shouldRedirectToWorkflowEngineUI
? `${makeMonitorBasePathname(organization.slug)}?alertsRedirect=true`
: `${baseUrl}/alerts/rules/`;

return (
<StickyBottomSection
id="issues-configure"
Expand All @@ -92,8 +103,8 @@ function ConfigureSection({baseUrl}: {baseUrl: string}) {
isSticky={isSticky}
>
<SecondaryNav.Item
to={`${baseUrl}/alerts/rules/`}
activeTo={`${baseUrl}/alerts/`}
to={alertsLink}
activeTo={alertsLink}
Copy link

@sentry sentry bot Nov 13, 2025

Choose a reason for hiding this comment

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

Bug: The activeTo prop for Alerts nav item is too specific, breaking highlighting for alert sub-pages.
Severity: HIGH | Confidence: 0.90

🔍 Detailed Analysis

The activeTo prop for the Alerts navigation item has been changed. For non-workflow-engine-ui users, it changed from activeTo={${baseUrl}/alerts/} to ${baseUrl}/alerts/rules/. For workflow-engine-ui users, it changed to /organizations/{slug}/monitors/?alertsRedirect=true. This change breaks navigation highlighting for alert sub-pages (e.g., /organizations/{slug}/issues/alerts/wizard/) because the isLinkActive function uses pathname.startsWith(toPathname) and the new activeTo values are too specific, no longer matching all alert-related routes.

💡 Suggested Fix

Revert activeTo to a broader path like ${baseUrl}/alerts/ or ensure activeTo covers all relevant alert sub-routes for both user types.

🤖 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/nav/secondary/sections/issues/issuesSecondaryNav.tsx#L106-L107

Potential issue: The `activeTo` prop for the Alerts navigation item has been changed.
For non-workflow-engine-ui users, it changed from `activeTo={`${baseUrl}/alerts/`}` to
`${baseUrl}/alerts/rules/`. For workflow-engine-ui users, it changed to
`/organizations/{slug}/monitors/?alertsRedirect=true`. This change breaks navigation
highlighting for alert sub-pages (e.g., `/organizations/{slug}/issues/alerts/wizard/`)
because the `isLinkActive` function uses `pathname.startsWith(toPathname)` and the new
`activeTo` values are too specific, no longer matching all alert-related routes.

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

Reference_id: 2663962

analyticsItemName="issues_alerts"
>
{t('Alerts')}
Expand Down
Loading