ui2: disable chonk opt in if user has exceeded subscriptions #100927
Conversation
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
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
Comment on lines
11
to
15
| if (exceededCategories.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| return <ChonkOptInBannerComponent collapsed="never" />; |
There was a problem hiding this comment.
Potential bug: The conditional logic for displaying the ChonkOptInBannerComponent is inverted, causing it to show only when subscription quotas are exceeded, which is the opposite of the intended behavior.
-
Description: The conditional logic to display the
ChonkOptInBannerComponentis inverted. The code currently checks ifexceededCategories.lengthis zero and returnsnull, which means the banner is only shown when a user has exceeded their subscription quotas. This directly contradicts the intended behavior stated in the pull request title, which is to disable the opt-in banner when quotas are exceeded. As a result, the banner promoting a new UI is displayed to users who may be dealing with billing issues, while it is hidden from users with no quota problems. -
Suggested fix: Invert the conditional logic to hide the banner when quotas are exceeded. The check should be
if (exceededCategories.length > 0)to returnnull, and the component should be rendered in the default case.
severity: 0.55, confidence: 0.98
Did we get this right? 👍 / 👎 to inform future reviews.
yeah feels like there should be some unified way to display notifications, probably with some central-ish registry so you can prioritize them. |
Co-authored-by: jonas.badalic <jonas.badalic@sentry.io>
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.
Fixes DE-276
What this PR does:
Some concerns and why I'm not sure if this is something we should land:
Imo, this makes is a bit nicer, but it's not a good solution at all, and we should revisit how notifications in our app work (least in the sidebar) and find a more complete solution. The independent messaging/signaling that each sidebar entry now does is not a scalable approach going forward