feat: Add Activated Alert Rules to alert rule index #69124

nhsiehgit · 2024-04-17T18:15:40Z

Duplicates the AlertRuleRow component to enable Activated Alert Rule rows.

Moves Alert Badge next to the rule status
Introduces a new "active state" to determine whether we are "ready to monitor" or "actively monitoring" states
cleans up the status logic

codecov · 2024-04-17T18:18:52Z

Bundle Report

Changes will increase total bundle size by 10.23kB ⬆️

Bundle name	Size	Change
app-webpack-bundle-array-push	26.24MB	10.23kB ⬆️

nhsiehgit · 2024-04-17T18:30:51Z

Changed the icons

saponifi3d

Left comments mostly focusing on the net new components.. i really recommend adding tests for the net new components as well. Maybe these could be built in a way that we could utilize the code with upcoming refactors as well?

saponifi3d · 2024-04-17T18:42:45Z

static/app/views/alerts/list/rules/activatedRuleRow.tsx

+ );
+ }
+
+ function renderAlertRuleStatus(): React.ReactNode {


I'd recommend decomposing this quite a bit.. you could really simplify this by pulling the alert status out as a different component.. then applying SOLID programming principles split this into it's most basic pieces.. each alert threshold state. Then have the AlertRuleStatus simply look at the state and decide which component to render. That would simplify the UI components, decouple your logic from presentation, and simplify testing.

saponifi3d · 2024-04-17T18:44:59Z

static/app/views/alerts/list/rules/activatedRuleRow.tsx

+ {!projectsLoaded && (
+ <LoadingIndicator
+ mini
+ style={{height: '24px', margin: 0, marginRight: 11}}


nit: make this a styled component, looks like the LoadingIndicator will take a class name and apply it correctly.

saponifi3d · 2024-04-17T18:54:49Z

static/app/views/alerts/list/rules/activatedRuleRow.tsx

+ <AlertName>
+ <div>{rule.name}</div>
+ </AlertName>


The inner div isn't needed here

Suggested change

<AlertName>

<div>{rule.name}</div>

</AlertName>

<AlertName>{rule.name}</AlertName>

saponifi3d · 2024-04-17T19:00:45Z

static/app/views/alerts/list/rules/activatedRuleRow.tsx

+const MenuItemWrapper = styled('div')`
+ display: flex;
+ align-items: center;
+ font-size: 13px;


Rather than hard coding it, we should use something from the theme.. there's fontSizeSmall which is 12px and feels like it maps to the intent of this a little more. If you have to keep it 13px, there's codeFontSize which also maps, but doesn't make as much sense from the naming perspective.

Suggested change

font-size: 13px;

font-size: ${p.theme.codeFontSize};

saponifi3d · 2024-04-17T19:01:21Z

static/app/views/alerts/list/rules/activatedRuleRow.tsx

+`;
+
+const Label = styled(TextOverflow)`
+ margin-left: 6px;


Suggested change

margin-left: 6px;

margin-left: $(space(0.75)};

saponifi3d · 2024-04-17T19:02:55Z

static/app/views/alerts/list/rules/activatedRuleRow.tsx

+ width: 24px;
+ height: 24px;


Suggested change

width: 24px;

height: 24px;

width: ${p.theme.iconSizes.lg};

height: ${p.theme.iconSizes.lg};

saponifi3d · 2024-04-17T19:12:18Z

static/app/views/alerts/list/rules/activatedRuleRow.tsx

+const FlexCenter = styled('div')`
+ display: flex;
+ align-items: center;
+`;
+
+const IssueAlertStatusWrapper = styled('div')`
+ display: flex;
+ align-items: center;
+ gap: ${space(1)};
+ line-height: 2;
+`;
+
+const AlertNameWrapper = styled('div')<{isIssueAlert?: boolean}>`
+ ${p => p.theme.overflowEllipsis}
+ display: flex;
+ align-items: center;
+ gap: ${space(2)};
+ ${p => p.isIssueAlert && `padding: ${space(3)} ${space(2)}; line-height: 2.4;`}
+`;


Might be nice to use static/app/components/profiling/flex.tsx and FlexContainer where you can here. I think you can cover most of the flex cases you have defined with that component. By reusing the component, it would likely cut down pretty substantially on the css payload of this component.

🤔
hmmm since most of this is just copy/pasta from the sister rules.tsx, i'm less inclined to rewrite the entire component

I can leave a note to come back to this though

sounds good - this pattern also already exists in the code so nbd.

saponifi3d · 2024-04-17T19:21:17Z

static/app/views/alerts/list/rules/activatedRuleRow.tsx

+ [rule]
+ );
+
+ function renderLatestActivation(): React.ReactNode {


Nit: recommend decomposing each of the render calls. If you're accessing things like the rule it'd be extra cool to use react-query to avoid prop drilling too.

hmmm how does react-query addres prop drilling?
Or did you mean context?

for now i think i'll keep these patterns to match rules.tsx 😅
I'll create a follow up ticket to clean both of these up though

ticket here: https://github.com/orgs/getsentry/projects/199/views/3?pane=issue&itemId=60130307

react-query uses context to store api data for you. (it also handles automatic updates and syncing data across all uses of the "store").

for example, if you wanted to fetch a specific rule you could do useRule(ruleId) -- then that hook uses the api react-query hook to fetch the data (which updates the value in the store for rule etc).

If you haven't used it before.. it's kinda a mind meld and we should def wait until later to update. i'm planning on using it for the API requests with project settings pages, so we could also take a look at it when that comes up.

scttcper · 2024-04-17T22:16:35Z

static/app/views/alerts/types.tsx

 type: CombinedAlertType.METRIC;
- latestIncident?: Incident | null;


why remove latestIncident? This property is returned when passing the latestIncident expand parameter https://github.com/getsentry/sentry/blob/master/static/app/views/alerts/list/rules/alertRulesList.tsx#L49 I assume the only reason this passes is because IssueAlert has latestIncident too which is probably not correct

oh i see its available on MetricRule

hmm that's probably wrong too lol

Ah - no it's because the MetricRule that this is extending already includes the latestIncident

saponifi3d

If you can, i'd try to add some tests for activatedRuleRow.tsx since that's a net new component. if possible, i think decomposing that class would be good follow up work.

saponifi3d · 2024-04-17T22:51:53Z

static/app/views/alerts/list/rules/activatedRuleRow.tsx

+ [rule]
+ );
+
+ function renderLatestActivation(): React.ReactNode {


react-query uses context to store api data for you. (it also handles automatic updates and syncing data across all uses of the "store").

for example, if you wanted to fetch a specific rule you could do useRule(ruleId) -- then that hook uses the api react-query hook to fetch the data (which updates the value in the store for rule etc).

If you haven't used it before.. it's kinda a mind meld and we should def wait until later to update. i'm planning on using it for the API requests with project settings pages, so we could also take a look at it when that comes up.

saponifi3d · 2024-04-17T22:52:51Z

static/app/views/alerts/list/rules/activatedRuleRow.tsx

+const FlexCenter = styled('div')`
+ display: flex;
+ align-items: center;
+`;
+
+const IssueAlertStatusWrapper = styled('div')`
+ display: flex;
+ align-items: center;
+ gap: ${space(1)};
+ line-height: 2;
+`;
+
+const AlertNameWrapper = styled('div')<{isIssueAlert?: boolean}>`
+ ${p => p.theme.overflowEllipsis}
+ display: flex;
+ align-items: center;
+ gap: ${space(2)};
+ ${p => p.isIssueAlert && `padding: ${space(3)} ${space(2)}; line-height: 2.4;`}
+`;


sounds good - this pattern also already exists in the code so nbd.

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 17, 2024

vercel bot deployed to Preview April 17, 2024 18:17 View deployment

vercel bot deployed to Preview April 17, 2024 18:26 View deployment

vercel bot deployed to Preview April 17, 2024 18:29 View deployment

nhsiehgit requested a review from saponifi3d April 17, 2024 18:31

vercel bot deployed to Preview April 17, 2024 18:32 View deployment

nhsiehgit requested a review from ceorourke April 17, 2024 18:33

saponifi3d reviewed Apr 17, 2024

View reviewed changes

vercel bot deployed to Preview April 17, 2024 21:10 View deployment

nhsiehgit marked this pull request as ready for review April 17, 2024 21:16

nhsiehgit requested a review from a team as a code owner April 17, 2024 21:16

nhsiehgit force-pushed the activated_alert_rule_index branch from ea16b2a to a2458bb Compare April 17, 2024 21:16

nhsiehgit requested review from scttcper and vuluongj20 April 17, 2024 21:17

vercel bot deployed to Preview April 17, 2024 21:19 View deployment

vercel bot deployed to Preview April 17, 2024 21:26 View deployment

vercel bot deployed to Preview April 17, 2024 21:44 View deployment

vercel bot deployed to Preview April 17, 2024 21:48 View deployment

scttcper reviewed Apr 17, 2024

View reviewed changes

saponifi3d approved these changes Apr 17, 2024

View reviewed changes

vercel bot deployed to Preview April 18, 2024 03:36 View deployment

nhsiehgit added 7 commits April 18, 2024 08:10

add activated alert rule row and fetch activated alerts

292129f

updated row status badge

5edcf36

move alert badge next to status

5a8db4f

remove link to alert details

e72d775

updated icons

3d54f15

nits

aa40fdb

update stub fixture

902efd6

nhsiehgit added 3 commits April 18, 2024 08:10

update test

4c830cd

expect renders activated rules

b0449f3

added test for activated status

09c8c78

nhsiehgit force-pushed the activated_alert_rule_index branch from 4287e0d to 09c8c78 Compare April 18, 2024 15:10

vercel bot deployed to Preview April 18, 2024 15:12 View deployment

nhsiehgit merged commit e4303d3 into master Apr 18, 2024
41 checks passed

nhsiehgit deleted the activated_alert_rule_index branch April 18, 2024 15:28

github-actions bot locked and limited conversation to collaborators May 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Activated Alert Rules to alert rule index #69124

feat: Add Activated Alert Rules to alert rule index #69124

nhsiehgit commented Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading

nhsiehgit commented Apr 17, 2024

saponifi3d left a comment

saponifi3d Apr 17, 2024

saponifi3d Apr 17, 2024

saponifi3d Apr 17, 2024

saponifi3d Apr 17, 2024

saponifi3d Apr 17, 2024

saponifi3d Apr 17, 2024

saponifi3d Apr 17, 2024

nhsiehgit Apr 17, 2024

saponifi3d Apr 17, 2024

saponifi3d Apr 17, 2024

nhsiehgit Apr 17, 2024

nhsiehgit Apr 17, 2024

saponifi3d Apr 17, 2024

scttcper Apr 17, 2024

scttcper Apr 17, 2024

scttcper Apr 17, 2024

nhsiehgit Apr 17, 2024

saponifi3d left a comment

saponifi3d Apr 17, 2024

saponifi3d Apr 17, 2024

		type: CombinedAlertType.METRIC;
		latestIncident?: Incident \| null;

feat: Add Activated Alert Rules to alert rule index #69124

feat: Add Activated Alert Rules to alert rule index #69124

Conversation

nhsiehgit commented Apr 17, 2024 • edited Loading

codecov bot commented Apr 17, 2024 • edited Loading

Bundle Report

nhsiehgit commented Apr 17, 2024

saponifi3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saponifi3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhsiehgit commented Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading