feat(aci): Send project slug with test fire action request#113127
feat(aci): Send project slug with test fire action request#113127
Conversation
The test notification endpoint accepts an optional projectSlug to control which project is shown in the notification. This picks a project from the connected monitors that the user has access to, so the test notification displays a relevant project instead of an arbitrary one. Also extracts ActionFilterBlock into its own file for clarity.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b589ff9. Configure here.
saponifi3d
left a comment
There was a problem hiding this comment.
just nits / questions.
| minHeight: '21px', | ||
| height: '21px', | ||
| }), | ||
| }} |
There was a problem hiding this comment.
just curious - if we're using styled on this component already, why are we inlining here? because the hardcoded 21px?
| minHeight: '21px', | ||
| height: '21px', |
There was a problem hiding this comment.
more of a general question; should try adding anything to the style guide that's close to these values?
i think 24 would probably be a decent value for these (if it still looks okay). that's be our base font size + 1/2 of base size.
... or should we just use 1.5rems here and kinda sorta align to the style guide? 🤔
not sure if it'd look off with the current design tho; but might be cool to use rems (if it's not an anti-pattern for us).
There was a problem hiding this comment.
yeah we probably should, I think the reason these overrides exist is because our form designs don't match some of the common form components that we have, though I'm hoping that it will be a bit better with the headless form hooks we have now
| const IfThenWrapper = styled(Flex)` | ||
| position: relative; | ||
| flex-direction: column; | ||
| gap: ${p => p.theme.space.md}; | ||
| border: 1px solid ${p => p.theme.tokens.border.primary}; | ||
| border-radius: ${p => p.theme.radius.md}; | ||
| padding: ${p => p.theme.space.lg}; | ||
| margin-top: ${p => p.theme.space.md}; | ||
|
|
||
| /* Only hide delete button when hover is supported */ | ||
| @media (hover: hover) { | ||
| &:not(:hover):not(:focus-within) { | ||
| .delete-condition-group { | ||
| ${p => p.theme.visuallyHidden} | ||
| } | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const DeleteButton = styled(Button)` | ||
| position: absolute; | ||
| top: ${p => p.theme.space.sm}; | ||
| right: ${p => p.theme.space.sm}; | ||
| `; |
There was a problem hiding this comment.
| const IfThenWrapper = styled(Flex)` | |
| position: relative; | |
| flex-direction: column; | |
| gap: ${p => p.theme.space.md}; | |
| border: 1px solid ${p => p.theme.tokens.border.primary}; | |
| border-radius: ${p => p.theme.radius.md}; | |
| padding: ${p => p.theme.space.lg}; | |
| margin-top: ${p => p.theme.space.md}; | |
| /* Only hide delete button when hover is supported */ | |
| @media (hover: hover) { | |
| &:not(:hover):not(:focus-within) { | |
| .delete-condition-group { | |
| ${p => p.theme.visuallyHidden} | |
| } | |
| } | |
| } | |
| `; | |
| const DeleteButton = styled(Button)` | |
| position: absolute; | |
| top: ${p => p.theme.space.sm}; | |
| right: ${p => p.theme.space.sm}; | |
| `; | |
| const DeleteButton = styled(Button)` | |
| position: absolute; | |
| top: ${p => p.theme.space.sm}; | |
| right: ${p => p.theme.space.sm}; | |
| `; | |
| const IfThenWrapper = styled(Flex)` | |
| position: relative; | |
| flex-direction: column; | |
| gap: ${p => p.theme.space.md}; | |
| border: 1px solid ${p => p.theme.tokens.border.primary}; | |
| border-radius: ${p => p.theme.radius.md}; | |
| padding: ${p => p.theme.space.lg}; | |
| margin-top: ${p => p.theme.space.md}; | |
| /* Only hide delete button when hover is supported */ | |
| @media (hover: hover) { | |
| &:not(:hover):not(:focus-within) { | |
| ${DeleteButton} { | |
| ${p => p.theme.visuallyHidden} | |
| } | |
| } | |
| } | |
| `; |
Could you do something like this then remove the className or are we trying to use that more to migrate away from styled components?
There was a problem hiding this comment.
yes it could certainly be written that way! There isn't much of a concerted effort right now to do one or the other, but we have discussed that targeting classnames or data attributes is probably better to make the future migration easier

Closes ISWF-2436
This is the frontend follow-up to #112859, which added
projectSlugsupport to the test fire action endpoint. This PR selects a connected project to pass for that value by addinguseTestNotificationProjectSlug()and a test. While doing so, I extracted ActionFilterBlock into its own file since it was getting kinda big.Tested it out and it works as expected. Before it selected the alphabetical first project, now it selects the one that's connected: