Skip to content

Conversation

@ameliahsu
Copy link
Member

redirects metric alert rules and uptime monitors to the new monitors views.

note that links to cron monitors do not get redirected at the moment since that would require a lookup from the monitorSlug to the detectorId

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Nov 19, 2025
@ameliahsu ameliahsu changed the title alert rules to detectors feat(aci): redirect alert rules to detectors Nov 19, 2025
@codecov
Copy link

codecov bot commented Nov 19, 2025

⚠️ JUnit XML file not found

The CLI was unable to find any JUnit XML files to upload.
For more help, visit our troubleshooting guide.

@ameliahsu ameliahsu marked this pull request as ready for review November 19, 2025 22:08
@ameliahsu ameliahsu requested review from a team as code owners November 19, 2025 22:08
Copy link
Contributor

@saponifi3d saponifi3d left a comment

Choose a reason for hiding this comment

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

i just generally have nits, but not super familiar with the routing layer here -- would recommend getting a second set of eyes for that stuff.

const {ruleId, detectorId} = useParams();

const shouldRedirect =
!user.isStaff && organization.features.includes('workflow-engine-ui');
Copy link
Contributor

Choose a reason for hiding this comment

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

curious about the !user.isStaff piece here, is that so we can continue with everything as is today w/o redirecting? might be nice to just have staff use the same experience; and this could be a nice forcing function for everyone to dog food this before we flip the LA switch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding the isStaff check makes it easier for us to compare the old vs. new experience while developing, so I'd like to keep this check for now

Comment on lines 178 to 182
alertType === 'crons'
? 'monitor_check_in_failure'
: alertType === 'uptime'
? 'uptime_domain_failure'
: null;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: recommend against having nested ternaries, makes readability even with these indentations a bit tricky.

Maybe something like:

const getDetectionType = (alertType: string): string => {
  switch(alertType) {
    case 'crons':
      return 'monitor_check_in_failure';
    case 'uptime':
      return 'uptime_domain_failure';
    default:
      return null;
  }
}

// in use
const detectorType = getDetectionType(alertType);

this makes it a little easier to quickly read, but also makes it a lot easier to add a 3rd type if we need to or anything.

{
path: 'details/:ruleId/',
component: make(() => import('sentry/views/alerts/rules/metric/details')),
component: WorkflowEngineRedirectToDetectorDetails,
Copy link
Member

Choose a reason for hiding this comment

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

import should still be lazy

* Base component for workflow engine redirects that require fetching
* workflow data from a metric alert rule before redirecting.
*/
function WorkflowEngineRedirectWithAlertRuleData({
Copy link
Member

Choose a reason for hiding this comment

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

can we make this a hook (or small HoCish wrapper for class components) and wrap the areas we want to redirect instead. Adding these to the route tree doesn't totally make sense if they aren't views

Copy link
Member

@malwilley malwilley Nov 20, 2025

Choose a reason for hiding this comment

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

This can't be a hook because it needs to render the loading state while the requests are in flight, but it could be an HoC. That would feel cleaner and wouldn't require changing the route tree

Copy link
Member

Choose a reason for hiding this comment

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

i think that's where i'm at yeah this shouldn't need to be in the route tree

{
path: 'details/:ruleId/',
component: make(() => import('sentry/views/alerts/rules/metric/details')),
component: withDetectorDetailsRedirect(
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this isn't completely lazy, you need to make a new file with a default export that is export default withDetectorDetailsRedirect(MetricDetails)

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.

5 participants