-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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.
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
Conversation
| const shouldRedirectToWorkflowEngineUI = | ||
| !user.isStaff && organization.features.includes('workflow-engine-ui'); |
There was a problem hiding this comment.
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
| to={alertsLink} | ||
| activeTo={alertsLink} |
There was a problem hiding this comment.
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
similar to #103129, redirects users with the
workflow-engine-uiflag who click on Issues > Alerts to the All Monitors list with a banner