feat(codeowners): Fix up Add Codeowners modal states

[NisanthanNanthakumar]

Objective

This PR has some UI cleanup for a nicer UX.

• Preview file should open in new tab, not in current tab
• There should be an empty state screen for users who click Add CodeOwners and have a source code integration. This is to let users setup a Code Mapping.

UI

[manuzope]

Question - what is a user supposed to do next after seeing that error message? Is there a place we can navigate them to so they can fix the problem?

[MeredithAnya]

I don't think we should say 'stack trace linking' since that's the feature, I think we should use 'code mapping' instead.

[MeredithAnya]

hmm I think im actually against using this endpoint for this. It kind of has nothing to do with the stacktrace it just happens to return the integrations filtered by stacktrace link, but I would argue we'd actually want it filtered by codeowners (once that is actually implemented). I think maybe the organizations/org_slug/integrations endpoint would be better here, and we can filter on the frontend by the integration.provider.features.

I think if we wanted we could even update that endpoint to take a features param if we wanted to filter on the backend, but I think there shouldn't normally be that many integrations anyway

[NisanthanNanthakumar]

@MeredithAnya from my understanding, the set of "codeowner" integrations is equivalent to the set of "stacktrace" integrations.

hmm I am generally against doing filtering logic in the frontend if the API is capable of returning the exact result. It slows down renders. I think this is a TODO item where we add the "codeowners" feature to the relevant integrations, update the organizations/org_slug/integrations to take a query param features and then update this line with the endpoint with query param

[MeredithAnya]

@NisanthanNanthakumar that's true for the moment, but it's totally possible down the road that we first add stack trace linking to another provider (Azure for example) first before adding codeowners, and in that case they may not be equivalent. There are also other params like the sourceUrl that make no sense in this context, and if you use the includeConfig=0 for the integrations endpoint, I think it should give you what you need

[NisanthanNanthakumar]

@MeredithAnya I agree that the response payload has extraneous info. However, using includeConfig=0 doesn't prevent the need from client-side filtering out integrations that do not support codeowners. If we want to use the organizations/org_slug/integrations endpoint, we need to add filtering query params. It is much better to do the filtering in the database rather than the client.

[manuzope]

I agree with @MeredithAnya here. Given that we're trying to get a list of integrations, it's more accurate to use the integrations endpoint rather than relying on the current assumption that stacktrace linking and codeowners work together.

WRT to client side filtering - I'd agree if we were talking about thousands or even hundreds of rows. But here, we're talking about low tens of rows so client side filtering works just fine imo. Yes, adding filtering to the endpoint would be useful but given we're talking about such few rows, client side filtering should not have a huge adverse affect on rendering times.

[NisanthanNanthakumar]

@manuzope saw this too late. I already added filtering by provider feature to the endpoint.

[MeredithAnya]

@manuzope @NisanthanNanthakumar I think we should be careful about adding CODEOWNERS to the IntegrationFeatures before we GA (we waited until GA for stack trace linking, this PR shows what we had in place prior https://github.com/getsentry/sentry/pull/24398/files). Also we should update the flag to include integrations- if that is going to be needed to gate it on plans

I think the client filtering is the way to go for now

[NisanthanNanthakumar]

I tested on getsentry and Code Owners doesn't show up as a feature tag for Github/Gitlab.

[MeredithAnya]

kind of a nit: it kind of feels weird that we tell them to add the missing user mappings if our error message just says the team names aren't associated. Maybe it could read something like

"Configure User or Team mappings for any missing associations."

[MeredithAnya]

Adding this here now is going to make it show up for everyone, and because it seems like since the feature flag isn't constructed with the integrations- prefix, it's going to show up as a free feature for everyone. Might want to confirm that by testing it out on getsentry though.

[MeredithAnya]

if you want this to be able to align with the feature gate it needs to be named the same so import-codeowners but actually now that i think about it, the feature gate should have been integrations-import-codeowners...

[MeredithAnya]

I think we'll still want to change the feature flag to have the integrations- prefix, but since the feature is not showing up in the integration directory yet I think that's okay to add after