-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(triage signals): Auto Trigger default settings change [feature flagged] #103992
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
4f44916 to
5390b35
Compare
5390b35 to
31df144
Compare
31df144 to
d811aa9
Compare
| }, | ||
| }); | ||
|
|
||
| // Toggle should be checked for new orgs | ||
| expect( | ||
| await screen.findByRole('checkbox', {name: /Auto-Trigger Fixes/i}) | ||
| ).toBeChecked(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Auto Create PR Setting', () => { |
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: Existing autofixAutomationTuning values are silently downgraded to 'medium' when the feature flag is enabled.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
When the triage-signals-v0 feature flag is enabled, existing autofixAutomationTuning values (e.g., 'high', 'low') are converted to a boolean true for UI display. If the user does not interact with the form, the getData function converts this true back to 'medium' upon submission, silently downgrading the original setting without user consent. This is evident from removed tests and new test fixtures explicitly using 'medium' to avoid this scenario.
💡 Suggested Fix
Ensure getData preserves existing autofixAutomationTuning string values when the toggle is displayed as checked, rather than unconditionally converting them to 'medium'.
🤖 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/views/settings/projectSeer/index.spec.tsx#L653-L722
Potential issue: When the `triage-signals-v0` feature flag is enabled, existing
`autofixAutomationTuning` values (e.g., `'high'`, `'low'`) are converted to a boolean
`true` for UI display. If the user does not interact with the form, the `getData`
function converts this `true` back to `'medium'` upon submission, silently downgrading
the original setting without user consent. This is evident from removed tests and new
test fixtures explicitly using `'medium'` to avoid this scenario.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3611850
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.
Yes that's the point of the feature.
| // - Existing orgs with 'off': shows unchecked, preserves their choice | ||
| const automationTuning = isTriageSignalsFeatureOn | ||
| ? (project.autofixAutomationTuning ?? 'off') !== 'off' | ||
| ? project.autofixAutomationTuning !== 'off' |
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.
Why does this branch return a boolean and the other branch return a string?
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.
If the feature flag is on "Auto-Trigger Fixes" is a toggle and if it's off it's a dropdown. Since it's a toggle when it's on I used bool here and I convert it to a string later. I could make it a string though a toggle being a string might be weird.
PR Details
triage-signals-v0feature flag is enabled, the "Auto-Trigger Fixes" toggle defaults to checked for new projects (undefined value). Setting persists on user interaction with the form.'off') will see the toggle unchecked, moving them over will be handled in the onboarding billing logic.