feat(onboarding): add messaging integration onboarding to project creation #79194

ameliahsu · 2024-10-16T16:53:52Z

adding an option on the project creation page to send alerts to a messaging integration (Slack, Discord, MS Teams, etc.)

email alert rules are still created as well (the checkbox to "notify via email" is selected by default, and can not be deselected)

if org has no integrations installed:

no-integration-fr.mov

*notice how the dropdowns to select an integration provider (slack, discord, ms teams) and workspace are disabled, because there is only 1 choice for each

if org has multiple integrations installed:

with-integration.mov

if the org is not on a team plan or above, the button is disabled:

if the user has selected "notify via integration" but they have not input a channel ID, the create project button is disabled:

error messages are displayed at the bottom (existing behavior):

codecov · 2024-10-16T16:58:09Z

Bundle Report

Changes will increase total bundle size by 17.37kB (0.06%) ⬆️. This is within the configured threshold ✅

Detailed changes

Bundle name	Size	Change
app-webpack-bundle-array-push	30.9MB	17.37kB (0.06%) ⬆️

codecov · 2024-10-16T17:01:26Z

Codecov Report

Attention: Patch coverage is 72.81553% with 28 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...s/projectInstall/issueAlertNotificationOptions.tsx	72.00%	14 Missing ⚠️
static/app/views/projectInstall/createProject.tsx	46.66%	8 Missing ⚠️
...s/projectInstall/messagingIntegrationAlertRule.tsx	81.25%	6 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #79194       +/-   ##
===========================================
- Coverage   88.51%   78.33%   -10.18%     
===========================================
  Files        3044     7126     +4082     
  Lines      189155   314565   +125410     
  Branches    31095    51375    +20280     
===========================================
+ Hits       167425   246429    +79004     
- Misses      15654    61686    +46032     
- Partials     6076     6450      +374

evanpurkhiser · 2024-10-16T17:44:39Z

static/app/views/projectInstall/createProject.tsx

+  const [alertNotificationAction, setAlertNotificationAction] = useState<
+    IssueAlertActionType[]
+  >([IssueAlertActionType.NOTIFY_EMAIL]);
+  const [alertNotificationProvider, setAlertNotificationProvider] = useState<
+    string | undefined
+  >(undefined);
+  const [alertNotificationIntegration, setAlertNotificationIntegration] = useState<
+    OrganizationIntegration | undefined
+  >(undefined);
+  const [alertNotificationChannel, setAlertNotificationChannel] = useState<
+    string | undefined
+  >(undefined);


Does all this state belong in the createProject component itself? Should this live somewhere more contained?

I thought about this too, but I wasn't sure where it would go because the alert rule is created here after the project itself has been created.

I also considered turning provider, integration, and channel into an object, but decided against it since I would just have to create individual setters for readability anyways- lmk if that wasn't the right approach tho

evanpurkhiser · 2024-10-16T17:48:55Z

static/app/views/projectInstall/createProject.tsx

+  const integrationAction = useMemo(() => {
+    const id = alertNotificationAction.find(
+      action => action !== IssueAlertActionType.NOTIFY_EMAIL
+    );
+    if (id === IssueAlertActionType.SLACK) {
+      return [
+        {
+          id: id,
+          workspace: alertNotificationIntegration?.id,
+          channel: alertNotificationChannel,
+        },
+      ];
+    }
+    if (id === IssueAlertActionType.DISCORD) {
+      return [
+        {
+          id: id,
+          server: alertNotificationIntegration?.id,
+          channel_id: alertNotificationChannel,
+        },
+      ];
+    }
+    if (id === IssueAlertActionType.MS_TEAMS) {
+      return [
+        {
+          id: id,
+          team: alertNotificationIntegration?.id,
+          channel: alertNotificationChannel,
+        },
+      ];
+    }
+    return undefined;
+  }, [alertNotificationAction, alertNotificationIntegration, alertNotificationChannel]);


Is the useMemo necessary here to make sure the object identity is stable?

evanpurkhiser · 2024-10-16T17:49:14Z

static/app/views/projectInstall/createProject.tsx

@@ -136,7 +185,28 @@ function CreateProject() {
              },
            }
          );
-          ruleId = ruleData.id;
+          ruleIds?.push(ruleData.id);


Why do you need the ?.

evanpurkhiser · 2024-10-16T17:51:15Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+  const {
+    alertNotificationAction: action,
+    alertNotificationProvider: provider,
+    setAlertNotificationAction: setAction,
+    setAlertNotificationIntegration: setIntegration,
+    setAlertNotificationProvider: setProvider,
+  } = useIssueAlertNotificationContext();


Looks like this context is only used once here. Do we really need a context?

Typically you want a context when you need state shared a cross indeterminately nested components where you wouldn't want to have to prop drill constantly

I wanted to avoid prop drilling from createProject, the context is being used in both issueAlertNotificationOptions and the nested messagingIntegrationAlertRule component. I think the context is also useful to avoid having to pass all of the states/setters through issueAlertOptions, which doesn't use them.

scttcper · 2024-10-16T17:57:56Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+    return selectedValues.some(v => v !== MultipleCheckboxOptions.EMAIL);
+  }, [selectedValues]);
+
+  const onChange = values => {


values is missing a type

Suggested change

const onChange = values => {

const onChange = values => {

scttcper · 2024-10-16T17:59:09Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+      </MultipleCheckbox>
+      {shouldRenderSetupButton && (
+        <SetupMessagingIntegrationButton
+          refetchConfigs={refetchConfigs}


could pass refetch directly since the refetchConfigs function doesn't add anything

Suggested change

refetchConfigs={refetchConfigs}

refetchConfigs={messagingIntegrationsQuery.refetch}

evanpurkhiser · 2024-10-16T18:16:24Z

static/app/views/projectInstall/createProject.tsx

-              ))}
+              {Object.keys(errors).map(key => {
+                const label =
+                  key === 'actions' ? 'Notify via integration' : startCase(key);


Suggested change

key === 'actions' ? 'Notify via integration' : startCase(key);

key === 'actions' ? t('Notify via integration') : startCase(key);

Also you probably don't want to just take the key and capitalize the first letter, instead what you probably want is a mapping of keys to translated strings

const errors: Record<typeof key, string> = { key1: t('Key1'), key2: t('Key2'), ... }

The Record<typeof key, string> is just an example, if the keys aren't actually know at type check time it could just be Record<string, string>

Something important to remmeber is that you can't give t() dynamic values like t(startCase(key)). This is because the way translations are extracted and sent to our translators is that we statically analyze our code and extract out the strings inside of the t("string") calls. So if you did t(dynamicVar) the extractor doesn't know what the string is and can't include it in the list of strings to translate.

I'm assuming they used startCase to be able to generically handle any errors that could occur- I'm not familiar enough with all of the possible errors to be able to make a record here 😕 I just wanted to handle this special "actions" case because this label is not clear where the error occurred (my guess is that this error was originally intended to only be shown on the Alert Creation page, where it's more clear)

scttcper · 2024-10-16T18:16:56Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+  },
+};
+
+enum MultipleCheckboxOptions {


can we use a const enum here if we're not using the index

Suggested change

enum MultipleCheckboxOptions {

const enum MultipleCheckboxOptions {

evanpurkhiser · 2024-10-16T18:17:55Z

static/app/views/projectInstall/issueAlertNotificationContext.tsx

+  alertNotificationAction: IssueAlertActionType[];
+  alertNotificationChannel: string | undefined;
+  alertNotificationIntegration: OrganizationIntegration | undefined;
+  alertNotificationProvider: string | undefined;
+  setAlertNotificationAction: (method: IssueAlertActionType[]) => void;
+  setAlertNotificationChannel: (channel: string | undefined) => void;
+  setAlertNotificationIntegration: (
+    integration: OrganizationIntegration | undefined
+  ) => void;
+  setAlertNotificationProvider: (provider: string | undefined) => void;


I think we can probably get rid of this context, but just for the future, I don't think these keys need to be nearly this verbose. They are already contained inside a context called IssueAlertNotificationContext so it would be easier to read these by just stripping off the alertNotification part.

scttcper · 2024-10-16T18:20:55Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+  const providersToIntegrations = useMemo(() => {
+    const map: {[key: string]: OrganizationIntegration[]} = {};
+    if (!messagingIntegrationsQuery.data) {
+      return {};


returning {} loses the type

Suggested change

return {};

return map;

evanpurkhiser · 2024-10-17T15:58:36Z

static/app/views/projectInstall/createProject.tsx

+        if (
+          organization.features.includes(
+            'messaging-integration-onboarding-project-creation'
+          ) &&
+          shouldCreateRule
+        ) {


Can this check with the flag be part of the createNotificationAction

You can just pass shouldCreateRule to createNotificationAction

in which case the function just becomes a no-op

evanpurkhiser · 2024-10-17T15:58:51Z

static/app/views/projectInstall/createProject.tsx

+          shouldCreateRule
+        ) {
+          const ruleData = await createNotificationAction({
+            api,


I don't think you'll need to pass api here, just call useApi inside the hook

When I tried to call useApi here I ran into an error about only calling hooks from the top-level component, unless I'm doing something wrong?

you want useApi inside the hook where you define createNotificationAction

evanpurkhiser · 2024-10-17T16:02:20Z

static/app/views/projectInstall/createProject.tsx

+    actions: alertNotificationActions,
+    provider,
+    integration,
+    channel,
+    setActions: setAlertNotificationActions,
+    setProvider,
+    setIntegration,
+    setChannel,


Do you actually need to destructure all these? Are you able to just have it look like this

Suggested change

actions: alertNotificationActions,

provider,

integration,

channel,

setActions: setAlertNotificationActions,

setProvider,

setIntegration,

setChannel,

...notificationProps,

evanpurkhiser · 2024-10-17T16:03:18Z

static/app/views/projectInstall/createProject.tsx

+            notificationProps={{
+              actions: alertNotificationActions,
+              channel,
+              integration,
+              provider,
+              setActions: setAlertNotificationActions,
+              setChannel,
+              setIntegration,
+              setProvider,
+            }}


If you make my change suggested here https://github.com/getsentry/sentry/pull/79194/files#r1805048688 could this just become

Suggested change

notificationProps={{

actions: alertNotificationActions,

channel,

integration,

provider,

setActions: setAlertNotificationActions,

setChannel,

setIntegration,

setProvider,

}}

notificationProps={notificationProps}

evanpurkhiser · 2024-10-17T16:07:48Z

static/app/views/projectInstall/createProject.spec.tsx

@@ -406,6 +427,13 @@ describe('CreateProject', function () {
      await userEvent.clear(screen.getByTestId('range-input'));
      expect(getSubmitButton()).toBeDisabled();

+      await userEvent.click(
+        screen.getByRole('checkbox', {
+          name: /notify via integration \(slack, discord, ms teams, etc\.\)/i,


do we actually need the regex here?

evanpurkhiser · 2024-10-17T16:08:43Z

static/app/views/projectInstall/createProject.tsx

+                const label =
+                  key === 'actions' ? t('Notify via integration') : startCase(key);


Let's definitely improve this and create a key → error text mapping if we can

evanpurkhiser · 2024-10-17T16:10:46Z

static/app/views/projectInstall/messagingIntegrationAlertRule.spec.tsx

+  // it('disables integration select when there is only one option', function () {
+  //   render(
+  //     getComponent({
+  //       alertNotificationIntegration: discordIntegrations[0],
+  //       alertNotificationProvider: 'discord',
+  //     })
+  //   );
+  //   screen.getByRole('text').click();
+  //   expect(screen.getByRole('textbox')).toBeDisabled();
+  // });


evanpurkhiser · 2024-10-17T16:11:18Z

static/app/views/projectInstall/messagingIntegrationAlertRule.tsx

+  }
+
+  return (
+    <div>


Extra div necissary?

evanpurkhiser · 2024-10-17T16:12:20Z

static/app/views/projectInstall/messagingIntegrationAlertRule.tsx

+        <Rule>
+          {t('Send')}
+          <InlineSelectControl
+            label="provider"


Label should be translated. Does this actually set the aria-label though? Do you need to use aria-label?

Yes I still need to fix this, this was probably why the test I commented out wasn't working 😵‍💫

Unfortunately the types on these controls are pretty broken 😥

evanpurkhiser · 2024-10-17T16:48:28Z

static/app/views/projectInstall/messagingIntegrationAlertRule.tsx

+        <Rule>
+          {t('Send')}
+          <InlineSelectControl
+            label="provider"
+            disabled={Object.keys(providersToIntegrations).length === 1}
+            value={provider}
+            options={providerOptions}
+            onChange={p => {
+              setProvider(p.value);
+              setIntegration(providersToIntegrations[p.value][0]);
+              setChannel('');
+            }}
+          />
+          {t('notification to the')}
+          <InlineSelectControl
+            label="integration"
+            role="select"
+            disabled={integrationOptions.length === 1}
+            value={integration}
+            options={integrationOptions}
+            onChange={i => setIntegration(i.value)}
+          />
+          {providerDetails[provider]?.label}
+          <InlineInput
+            // label="channel"
+            type="text"
+            value={channel || ''}
+            placeholder={providerDetails[provider]?.placeholder}
+            onChange={(e: React.ChangeEvent<HTMLInputElement>) =>
+              setChannel(e.target.value)
+            }
+          />
+        </Rule>


This whole thing actually needs to be using tct (translate string with component interpolation).

The reason for this is because the english sentence structure is not the same as other language sentence structures.

Right now this might produce something that looks like

Send [Slack] notification to the [my slack] workspace to [#alerts]

But now if we take that sentence and look at what it looks like in Japanese you get

[Slack]通知を[my slack]ワークスペースに[#alerts]に送信します

Right away you can notice that the ordering is a little bit different.

In Japanese the sentence uses Subject Object Verb ordering, unlike english that uses Subject Verb Object.

In Japanese "[Slack]通知" is "Slack Notification", then at the end we have "送信します" which roughly means "send / transmit".

However the way this is written right now, the sentence structure of this sentence and these components cannot be moved around like that. In fact, it's even worse because the translator is only given two strings to translate

Send and notification to the. Out of context in other languages it's impossible to know what additional gramatical markers should be include.

Now, if you were to change this to use tct we can represent the whole sentence.

It's a little tricky for this example, because providerDetails[provider]?.label is actually also part of the sentence and should be translated. So what we probably want is that instead of having the provider configuration look like this:

slack: { name: 'Slack', action: IssueAlertActionType.SLACK, label: 'workspace to', placeholder: 'channel, e.g. #critical', }, discord: { name: 'Discord', action: IssueAlertActionType.DISCORD, label: 'server in the channel', placeholder: 'channel ID or URL', },

It should look closer to

slack: { name: t('Slack'), action: IssueAlertActionType.SLACK, placeholder: t('channel, e.g. #critical'), makeSentence: ({providerName, integrationName, target}) => tct( 'Send [providerName] notification to the [integrationName] workspace to [target]', { providerName, integrationName, target } ) }, discord: { name: t('Discord'), action: IssueAlertActionType.DISCORD, placeholder: t('channel ID or URL'), makeSentence: ({providerName, integrationName, target}) => tct( 'Send [providerName] notification to the [integrationName] server in the channel [target]', { providerName, integrationName, target } ) },

now the translator will have the string

Send [providerName] notification to the [integrationName] workspace to [target]

If the translator translates this to Japanese it will look like this:

[providerName]通知を[integrationName]ワークスペース、[target]に送信します

And for discord

Send [providerName] notification to the [integrationName] server in the channel [target]
[providerName]通知をチャネル[target]の[integrationName]サーバーに送信します

(These translations probably aren't perfect @corps can check me)

See how they were able to move around the object and the verb. That wasn't possible before.

Now this code looks like

Suggested change

<Rule>

{t('Send')}

<InlineSelectControl

label="provider"

disabled={Object.keys(providersToIntegrations).length === 1}

value={provider}

options={providerOptions}

onChange={p => {

setProvider(p.value);

setIntegration(providersToIntegrations[p.value][0]);

setChannel('');

}}

/>

{t('notification to the')}

<InlineSelectControl

label="integration"

role="select"

disabled={integrationOptions.length === 1}

value={integration}

options={integrationOptions}

onChange={i => setIntegration(i.value)}

/>

{providerDetails[provider]?.label}

<InlineInput

// label="channel"

type="text"

value={channel || ''}

placeholder={providerDetails[provider]?.placeholder}

onChange={(e: React.ChangeEvent<HTMLInputElement>) =>

setChannel(e.target.value)

}

/>

</Rule>

<Rule>

{providerDetails[provider]?.makeSentence({

providerName: (

<InlineSelectControl

label="provider"

disabled={Object.keys(providersToIntegrations).length === 1}

value={provider}

options={providerOptions}

onChange={p => {

setProvider(p.value);

setIntegration(providersToIntegrations[p.value][0]);

setChannel('');

}}

/>

),

integrationName: (

<InlineSelectControl

label="integration"

role="select"

disabled={integrationOptions.length === 1}

value={integration}

options={integrationOptions}

onChange={i => setIntegration(i.value)}

/>

),

target: (

<InlineInput

// label="channel"

type="text"

value={channel || ''}

placeholder={providerDetails[provider]?.placeholder}

onChange={(e: React.ChangeEvent<HTMLInputElement>) =>

setChannel(e.target.value)

}

/>

),

}) ?? tct('Select via [providerName]', {providerName: (...)})}

</Rule>

This can now be correctly translated across languages

ameliahsu · 2024-10-17T18:48:05Z

static/app/views/projectInstall/createProject.tsx

+  const keyToErrorText = {
+    actions: t('Notify via integration'),
+  };
+


I haven't been able to cause any other error messages, so right now the key --> error text mapping is very sparse 😔

Can we trace back what they could be?

evanpurkhiser · 2024-10-17T19:19:44Z

static/app/views/projectInstall/createProject.tsx

+        const ruleData = await createNotificationAction({
+          shouldCreateRule,
+          name,
+          projectSlug: projectData.slug,
+          conditions,
+          actionMatch,
+          frequency,
+        });
+        if (ruleData) {
+          ruleIds.push(ruleData.id);


Nice!! Much cleaner!

evanpurkhiser · 2024-10-17T19:20:23Z

static/app/views/projectInstall/createProject.tsx

@@ -407,7 +449,10 @@ function CreateProject() {
            <Alert type="error">
              {Object.keys(errors).map(key => (
                <div key={key}>
-                  <strong>{startCase(key)}</strong>: {errors[key]}
+                  <strong>
+                    {keyToErrorText[key] ? keyToErrorText[key] : startCase(key)}


Suggested change

{keyToErrorText[key] ? keyToErrorText[key] : startCase(key)}

{keyToErrorText[key] ?? startCase(key)}

evanpurkhiser · 2024-10-17T19:22:02Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+      >
+        <Wrapper>
+          <MultipleCheckbox.Item value={MultipleCheckboxOptions.EMAIL} disabled>
+            Notify via email


Suggested change

Notify via email

{t('Notify via email')}

evanpurkhiser · 2024-10-17T19:22:15Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+          {!shouldRenderSetupButton && provider && (
+            <div>
+              <MultipleCheckbox.Item value={MultipleCheckboxOptions.INTEGRATION}>
+                Notify via integration (Slack, Discord, MS Teams, etc.)


Suggested change

Notify via integration (Slack, Discord, MS Teams, etc.)

{t('Notify via integration (Slack, Discord, MS Teams, etc.)')}

evanpurkhiser · 2024-10-17T19:22:40Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+                <MessagingIntegrationAlertRule
+                  notificationProps={notificationProps}
+                  providersToIntegrations={providersToIntegrations}
+                />


Should this be inside of the MultipleCheckbox container?

I put it outside to avoid this centering:

because ideally we want the integration alert configuations to be underneath the checkbox like this:

This is probably fine, I was expecting that multiple checkbox was actually rendering a list, but it looks like it's actually just divs.

If it WAS rendering a list and this looked like

<ul> <li> <input type="checkbox" /> Label 1 </li> <li> <input type="checkbox" /> Notify via Integration... </li> <div> Send .... </div> </ul>

This would actually be incorrect html. See

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/ul#technical_summary

Permitted content: Zero or more <li>, <script> and <template> elements.

You're not allowed to have div elements inside a ul.

That said browsers render it fine. It's just not "proper" semantic HTML.

This can have downstream effects on things like screen readers for blind users. But that 's already a problem here since we're using unsemantic HTML to render this list of check boxes (just divs) instead of using a real list.

evanpurkhiser · 2024-10-17T19:25:42Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+  discord: {
+    name: 'Discord',
+    action: IssueAlertActionType.DISCORD,
+    placeholder: 'channel ID or URL',


Suggested change

placeholder: 'channel ID or URL',

placeholder: t('channel ID or URL'),

forgot these

evanpurkhiser · 2024-10-17T19:26:03Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+      ),
+  },
+  discord: {
+    name: 'Discord',


Suggested change

name: 'Discord',

name: t('Discord'),

evanpurkhiser · 2024-10-17T19:26:16Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+  msteams: {
+    name: 'MSTeams',
+    action: IssueAlertActionType.MS_TEAMS,
+    placeholder: 'channel ID',


Suggested change

placeholder: 'channel ID',

placeholder: t('channel ID'),

evanpurkhiser · 2024-10-17T19:27:34Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+      ),
+  },
+  msteams: {
+    name: 'MSTeams',


Is it called MSTeams or just Teams? Was pretty sure this product is just called Teams

We really would want a translator note here, but as mentioned before, we don't support that so doing this might end up being confusing for our translators :(

Suggested change

name: 'MSTeams',

name: t('Teams'),

maybe better is actually

Suggested change

name: 'MSTeams',

name: t('MS Teams'),

I believe it's called MS Teams, or at least that's what we have everywhere else in our codebase

evanpurkhiser · 2024-10-17T19:28:43Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+        return undefined;
+      }
+
+      let integrationAction;


This is missing a type

evanpurkhiser · 2024-10-17T19:29:25Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+export default function IssueAlertNotificationOptions(
+  notificationProps: IssueAlertNotificationProps
+) {
+  const organization = useOrganization();
+  const {actions, provider, setActions, setIntegration, setProvider} = notificationProps;


Suggested change

export default function IssueAlertNotificationOptions(

notificationProps: IssueAlertNotificationProps

) {

const organization = useOrganization();

const {actions, provider, setActions, setIntegration, setProvider} = notificationProps;

export default function IssueAlertNotificationOptions(

{actions, provider, setActions, setIntegration, setProvider}: IssueAlertNotificationProps

) {

const organization = useOrganization();

Ah I wrote it this way bc notificationProps still needs to be passed to MessagingIntegrationAlertRule here

evanpurkhiser · 2024-10-17T19:30:24Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+  );
+
+  const providersToIntegrations = useMemo(() => {
+    const map: {[key: string]: OrganizationIntegration[]} = {};


Suggested change

const map: {[key: string]: OrganizationIntegration[]} = {};

const map: Record<string, OrganizationIntegration[]> = {};

There's no real difference here, but one is easier to read

evanpurkhiser · 2024-10-17T19:33:57Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+      setProvider(firstProvider);
+
+      const firstIntegration = providersToIntegrations[firstProvider][0];
+      setIntegration(firstIntegration);


Do we actually need this state if we can just derive all of this?

or is this just for setting up defaults?

If it's for setting up defaults I think you could probably do this in the constructor functions of the set states in the hook. Assuming we move the api calls into that hook

evanpurkhiser · 2024-10-17T19:35:22Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+  const organization = useOrganization();
+  const {actions, provider, setActions, setIntegration, setProvider} = notificationProps;
+
+  const messagingIntegrationsQuery = useApiQuery<OrganizationIntegration[]>(


Any reason this stuff shouldn't be part of the useCreateNotificationAction hook?

evanpurkhiser · 2024-10-17T19:37:30Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+  const shouldRenderSetupButton = useMemo(() => {
+    return messagingIntegrationsQuery.data?.every(i => i.status !== 'active');
+  }, [messagingIntegrationsQuery.data]);


I don't think we need a useMemo here. This computation is not expensive and can just be run on render

evanpurkhiser · 2024-10-17T19:39:12Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+  const shouldRenderNotificationConfigs = useMemo(() => {
+    return actions.some(v => v !== MultipleCheckboxOptions.EMAIL);
+  }, [actions]);


Same here, no need for useMemo.

use memo should really only be used when we

need a stable reference to a value where it would change if it was re-computed on every render: eg if we had

const something = actions.map(...)

This would create a new object every time, and we would want to put it in a memo to stop that from happening

OR

The computation is expensive. This is actually quite rare.

evanpurkhiser · 2024-10-17T19:39:34Z

static/app/views/projectInstall/issueAlertNotificationOptions.tsx

+  }
+
+  return (
+    <div>


Is this extra div needed? Can it just be a fragment

evanpurkhiser · 2024-10-17T19:43:09Z

static/app/views/projectInstall/messagingIntegrationAlertRule.tsx

+const RuleWrapper = styled('div')`
+  padding: ${space(1)};
+  background-color: ${p => p.theme.backgroundSecondary};
+  border-radius: ${p => p.theme.borderRadius};
+`;
+
+const Rule = styled('div')`
+  display: flex;
+  align-items: center;
+  gap: ${space(1)};
+`;


Do We need both Rule and RuleWrapper? Seems like we could combine them?

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

evanpurkhiser reviewed Oct 16, 2024

View reviewed changes

scttcper reviewed Oct 16, 2024

View reviewed changes

evanpurkhiser reviewed Oct 16, 2024

View reviewed changes

scttcper reviewed Oct 16, 2024

View reviewed changes

evanpurkhiser reviewed Oct 16, 2024

View reviewed changes

scttcper reviewed Oct 16, 2024

View reviewed changes

ameliahsu added 2 commits October 16, 2024 15:33

first draft

8aa9f38

use hook and no context

43e4ea7

ameliahsu force-pushed the mia/proj-creation/fe branch from edbf08e to 43e4ea7 Compare October 16, 2024 23:28

vercel bot deployed to Preview October 16, 2024 23:32 View deployment

dont create alert rule if set up later option is chosen

565c186

vercel bot deployed to Preview October 17, 2024 00:33 View deployment

SWITCH STATEMENT YAY

35032f2

vercel bot deployed to Preview October 17, 2024 00:43 View deployment

one less useMemo

a9db89b

vercel bot deployed to Preview October 17, 2024 07:41 View deployment

evanpurkhiser reviewed Oct 17, 2024

View reviewed changes

translation + other fixes

68d0db2

ameliahsu commented Oct 17, 2024

View reviewed changes

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

ameliahsu marked this pull request as ready for review October 17, 2024 18:48

ameliahsu requested a review from evanpurkhiser October 17, 2024 18:49

evanpurkhiser reviewed Oct 17, 2024

View reviewed changes

	refetchConfigs={refetchConfigs}
	refetchConfigs={messagingIntegrationsQuery.refetch}

	key === 'actions' ? 'Notify via integration' : startCase(key);
	key === 'actions' ? t('Notify via integration') : startCase(key);

	enum MultipleCheckboxOptions {
	const enum MultipleCheckboxOptions {

		const label =
		key === 'actions' ? t('Notify via integration') : startCase(key);

	{keyToErrorText[key] ? keyToErrorText[key] : startCase(key)}
	{keyToErrorText[key] ?? startCase(key)}

	Notify via integration (Slack, Discord, MS Teams, etc.)
	{t('Notify via integration (Slack, Discord, MS Teams, etc.)')}

	placeholder: 'channel ID or URL',
	placeholder: t('channel ID or URL'),

	const map: {[key: string]: OrganizationIntegration[]} = {};
	const map: Record<string, OrganizationIntegration[]> = {};

feat(onboarding): add messaging integration onboarding to project creation #79194

Are you sure you want to change the base?

feat(onboarding): add messaging integration onboarding to project creation #79194

Conversation

ameliahsu commented Oct 16, 2024 • edited Loading

codecov bot commented Oct 16, 2024 • edited Loading

Bundle Report

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

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

ameliahsu Oct 16, 2024 • edited Loading

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

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

evanpurkhiser Oct 17, 2024 • edited Loading

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

ameliahsu Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

evanpurkhiser Oct 17, 2024 • edited Loading

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

evanpurkhiser Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ameliahsu commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

ameliahsu Oct 16, 2024 •

edited

Loading

evanpurkhiser Oct 17, 2024 •

edited

Loading

ameliahsu Oct 17, 2024 •

edited

Loading

evanpurkhiser Oct 17, 2024 •

edited

Loading

evanpurkhiser Oct 17, 2024 •

edited

Loading