-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(aci): add sentry app action details to conditions panel #93193
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
@@ -1,6 +1,7 @@ | |||
export interface Action { | |||
config: { | |||
target_type: ActionTarget; | |||
sentry_app_identifier?: SentryAppIdentifier; |
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.
Are these keys something we are going to try to camelcase at some point?
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.
we definitely should camelcase them at some point, but the api currently returns them in snakecase 😞
const name = handler?.sentryApp?.name; | ||
const title = handler?.sentryApp?.title; | ||
|
||
return title || name; |
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.
Do we need a fallback here? This will render undefined
if neither title or name are available
import {useActionNodeContext} from 'sentry/views/automations/components/actionNodes'; | ||
import {SentryAppActionSettingsButton} from 'sentry/views/automations/components/actions/sentryAppSettingsButton'; | ||
|
||
export function SentryAppDetails({handler}: {handler: ActionHandler}) { | ||
const name = handler?.sentryApp?.name; |
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.
What is the difference between a name and a title? Might be good context to add somewhere for future code readers
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.
good point i've added a comment to the type, name
is the name of the sentry app while title
is the action that the sentry app performs (for example "Create a Linear issue")
name: string; | ||
status: number; | ||
settings?: Record<string, any>; | ||
// title represents the action being performed by the SentryApp |
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.
If you put this in a multiline comment block (with the /* * */
), IDEs will pick up on it when you start typing out properties which is helpful. I usually do that when documenting type properties like this
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.
i got a linting error when i did this 😳
Expected multiple line comments instead of a block comment.eslint[multiline-comment-style](https://eslint.org/docs/latest/rules/multiline-comment-style)
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.
You won't get a lint error if you do e.g.:
/**
* The response status code
*/
status: Response['status'];
added logic to look up the correct action handler depending on the sentry app identifier provided by the action