Skip to content

Conversation

@ameliahsu
Copy link
Member

similar to #103129, redirects users with the workflow-engine-ui flag who click on Issues > Alerts to the All Monitors list with a banner

Screenshot 2025-11-13 at 11 56 52 AM

@ameliahsu ameliahsu requested review from a team as code owners November 13, 2025 19:58
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Nov 13, 2025
Comment on lines +91 to +92
const shouldRedirectToWorkflowEngineUI =
!user.isStaff && organization.features.includes('workflow-engine-ui');
Copy link

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

Comment on lines 106 to 107
to={alertsLink}
activeTo={alertsLink}
Copy link

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

@ameliahsu ameliahsu merged commit 2faaca2 into master Nov 15, 2025
48 checks passed
@ameliahsu ameliahsu deleted the mia/aci/redirect-alerts-nav branch November 15, 2025 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants