feat(aci): redirect alerts nav to monitors#103325
Conversation
| const shouldRedirectToWorkflowEngineUI = | ||
| !user.isStaff && organization.features.includes('workflow-engine-ui'); |
There was a problem hiding this comment.
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
| to={alertsLink} | ||
| activeTo={alertsLink} |
There was a problem hiding this comment.
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
similar to #103129, redirects users with the
workflow-engine-uiflag who click on Issues > Alerts to the All Monitors list with a banner