fix(iswf): Surfaces linked issues for Sentry Apps with no UI components#113372
Open
GabeVillalobos wants to merge 1 commit intomasterfrom
Open
fix(iswf): Surfaces linked issues for Sentry Apps with no UI components#113372GabeVillalobos wants to merge 1 commit intomasterfrom
GabeVillalobos wants to merge 1 commit intomasterfrom
Conversation
Comment on lines
+116
to
+118
| const installations = sentryAppInstallations.filter( | ||
| i => i.app.slug === externalIssue.serviceType | ||
| ); |
Contributor
There was a problem hiding this comment.
Bug: If a Sentry App's slug is changed, previously linked external issues for that app will no longer be displayed because the lookup relies on the original slug.
Severity: LOW
Suggested Fix
To ensure a stable association, use an immutable identifier for the Sentry App, such as its ID, for the lookup instead of the mutable slug. This would prevent the connection from breaking if the slug is ever modified.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
static/app/components/group/externalIssuesList/hooks/useSentryAppExternalIssues.tsx#L116-L118
Potential issue: The `PlatformExternalIssue.service_type` field is set to the Sentry
App's slug at the time an external issue is created. The frontend code later matches
this stored `service_type` against the current slug of installed Sentry Apps. If a
Sentry App's slug is changed after issues have been linked, the comparison will fail.
This causes previously linked external issues to silently disappear from the UI, as they
can no longer be associated with the correct Sentry App installation.
Did we get this right? 👍 / 👎 to inform future reviews.
Member
Author
There was a problem hiding this comment.
Already called this out in the description. While true, displaying all issues, including those for uninstalled sentry apps seems incorrect.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Shows linked Sentry App issues, even if now UI components for Link or Create are defined. As before, these linked issues will only be visible for Sentry Apps that are installed. Unlinking functionality will still work the same as before.
Some notable caveats:
service_typefield on thePlatformExternalIssuemodel, which can be a little funky if a Sentry App slug is changed, meaning issues can disappear if this occursResolves #108103