-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
chore(detectors): Update feedback for experimental issues #99550
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #99550 +/- ##
=======================================
Coverage 81.22% 81.22%
=======================================
Files 8590 8590
Lines 380349 380345 -4
Branches 24128 24127 -1
=======================================
- Hits 308944 308942 -2
+ Misses 71042 71040 -2
Partials 363 363 |
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.
nice! would it be possible to mark groups with an experimental
flag that gets returned to the client which we could rely on, vs hardcoding each issue type in the UI?
} | ||
> | ||
{t('Give Feedback')} | ||
</Button> |
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.
Bug: Query Injection Documentation Link Missing
The refactoring removed the dedicated "Learn more about the query injection issue" link from the issue details header. Query injection vulnerabilities previously always displayed this specific documentation link. Now, that link is gone, and the header instead shows either a generic feedback button (if available) or a different button, removing important educational content.
organization.features.includes( | ||
'organizations:experimental-n-plus-one-api-detector-rollout' | ||
)); |
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 code calls .includes()
on organization.features
, which can be undefined
from an API response, leading to a TypeError and component crash.
-
Description: The code directly accesses
organization.features.includes()
to check for a feature flag. Although the TypeScript type fororganization.features
isstring[]
, repository-specific documentation explicitly warns that API responses may omit this field, resulting in anundefined
value on the client. This will cause aTypeError: Cannot read property 'includes' of undefined', which will crash the issue details header component. The existence of defensive checks like
organization.features?.includes()` in other parts of the codebase confirms this is a known, real-world scenario that needs to be handled. -
Suggested fix: Add a defensive check before calling
.includes()
. Use optional chaining (organization.features?.includes(...)
) or the nullish coalescing operator ((organization.features ?? []).includes(...)
) to provide a default empty array iffeatures
is undefined.
severity: 0.65, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
i think that would be great - definitely something I'll take a look at when we cleanup some of the code in the backend |
updates the feedback button for detected issues on the issue details page to support N+1 API Calls.