-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(eco): add pending deletion state for sentry app installs #101125
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
chore(eco): add pending deletion state for sentry app installs #101125
Conversation
(existingData = []) => | ||
existingData.map(i => | ||
i.app.slug === sentryApp.slug ? {...i, status: 'pending_deletion'} : i | ||
) | ||
); |
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.
Potential bug: The frontend optimistically sets an installation's status to 'pending_deletion'
, but the backend immediately deletes the record, causing state inconsistency.
-
Description: When a user uninstalls a Sentry App, the frontend sends a DELETE request and then optimistically updates the local state of the installation to have a status of
'pending_deletion'
. However, the backend API endpoint immediately and synchronously deletes the installation record from the database, rather than marking it for asynchronous deletion. The backend does not support a'pending_deletion'
status. This mismatch causes the frontend to display an installation as 'Pending Deletion' when it has already been removed from the backend, leading to state inconsistency and potential errors on subsequent data fetches for that installation. -
Suggested fix: Align the frontend and backend behavior. Either the backend should be updated to support asynchronous deletion with a
pending_deletion
status, or the frontend should be changed to immediately remove the uninstalled application from its local state upon a successful DELETE API response, instead of setting the'pending_deletion'
status.
severity: 0.65, confidence: 0.98
Did we get this right? 👍 / 👎 to inform future reviews.
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.
bestie you don't even knowwwww
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.
read my PR descriptionnnnnn
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.
niceeeeee
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.
lol added an actual comment but would be nice to see an image
|
||
export const getSentryAppInstallStatus = (install: SentryAppInstallation | undefined) => { | ||
if (install) { | ||
if (install && install.status !== 'pending_deletion') { |
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.
Have we updated the APIs to return sentry apps that are pending deletion? Also, is there an enum we can reference for object status? Seems fairly generic.
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.
The status looks like this https://github.com/getsentry/sentry/pull/101125/files#diff-b2f09d35e3f0947a8312823f96c548de7361dc682c81b6f9c272b7a97408baf4R270
Everything is returned unless it has been "soft deleted", in which case the object manager will not return it
Merge after #100930
When deleting a Sentry app install, we queue it for deletion in the above PR, so we should show this state to the FE.
After the DELETE call is successful, update the FE state of the current installation to
pending_deletion
and also update buttons and labels.