-
Notifications
You must be signed in to change notification settings - Fork 84
Update Feature Flags for Release #6880
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
86c5253 to
941bf85
Compare
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.
Greptile Overview
Greptile Summary
This PR transitions the LLM classifier feature from alpha to beta by renaming the alphaFullActionCenter flag to llmClassifier. The changes implement flexible navigation routing where the Action Center appears when either webMonitor OR llmClassifier flags are enabled, while legacy pages (Activity, Data detection, Data discovery) are hidden when llmClassifier is enabled.
Key changes:
- Renamed feature flag with clearer, beta-appropriate naming
- Added
requiresAnyFlagandhidesIfFlagnavigation properties for flexible route visibility - Added backend configuration check (
detection_discovery.llm_classifier_enabled) to disable the LLM classifier toggle when backend doesn't support it - Updated monitor filtering logic to show website monitors only with
webMonitorflag and datastore/infrastructure monitors only withllmClassifierflag - Applied sentence case capitalization throughout UI labels
One minor logic issue found in ConfigureMonitorForm.tsx where the initial value for use_llm_classifier is calculated using && llmClassifierEnabled, which will incorrectly show false for monitors that were previously configured with LLM classifier enabled when the backend flag is now disabled.
Confidence Score: 4/5
- This PR is safe to merge with one minor logic issue that should be addressed
- The PR is well-structured with comprehensive feature flag refactoring. The logic issue in ConfigureMonitorForm.tsx line 175 is not critical but could cause user confusion when a monitor was previously configured with LLM classifier enabled but the backend flag is now disabled - the form would show the toggle as off when it should show the actual saved state.
- ConfigureMonitorForm.tsx:175 - review initial value logic for use_llm_classifier
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| clients/admin-ui/src/features/common/nav/nav-config.tsx | 5/5 | Added flexible flag routing with requiresAnyFlag and hidesIfFlag properties, updated routes to use new llmClassifier flag |
| clients/admin-ui/src/pages/data-discovery/action-center/index.tsx | 5/5 | Updated monitor filtering to show website monitors only if webMonitor is enabled and non-website monitors only if llmClassifier is enabled |
| clients/admin-ui/src/features/integrations/configure-monitor/ConfigureMonitorForm.tsx | 4/5 | Added backend config check for LLM classifier, disabled toggle with tooltip when backend doesn't support it, updated text to sentence case. Minor issue with initial value logic when backend flag is disabled. |
7 files reviewed, 1 comment
clients/admin-ui/src/features/integrations/configure-monitor/ConfigureMonitorForm.tsx
Outdated
Show resolved
Hide resolved
7787418 to
b4daed2
Compare
b4daed2 to
7c918fb
Compare
6e8f5e0 to
23f8642
Compare
23f8642 to
0587b00
Compare
0587b00 to
0e39916
Compare
92f0cdf to
c68bcc4
Compare
| route.hidesIfFlag && | ||
| flags && | ||
| flags[route.hidesIfFlag] && | ||
| !window?.Cypress |
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.
This was necessary to keep old tests from failing
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.
Greptile Overview
Greptile Summary
This PR renames the alphaFullActionCenter feature flag to llmClassifier with a more precise description that reflects its beta status. The flag now specifically controls LLM classifier functionality for Helios datastore monitors and the updated Action Center UI.
Key changes:
- Action Center navigation now appears when either
webMonitorORllmClassifierflags are enabled (new OR logic viarequiresAnyFlag) - Legacy pages (Activity, Data detection, Data discovery) are hidden when
llmClassifieris enabled (newhidesIfFlagproperty) - Added backend configuration check (
detection_discovery.llm_classifier_enabled) to disable the LLM classifier toggle with a helpful tooltip when the server doesn't support it - Removed the Prompt Template field from monitor configuration as it's no longer needed
- Updated monitor list filtering to correctly show website monitors only when
webMonitoris on, and datastore/infrastructure monitors whenllmClassifieris on
The implementation cleanly separates web monitoring concerns from LLM classifier functionality, allowing the Action Center to serve both features independently.
Confidence Score: 4/5
- This PR is safe to merge with one minor logic consideration around preserved state handling
- The refactoring is well-structured with proper flag separation and comprehensive test coverage. One concern exists around initial value logic in ConfigureMonitorForm where a monitor previously configured with LLM classifier enabled would show as disabled in the UI if the server flag is off, potentially causing confusion.
- Pay attention to clients/admin-ui/src/features/integrations/configure-monitor/ConfigureMonitorForm.tsx for the initial value logic
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| clients/admin-ui/src/features/common/nav/nav-config.tsx | 5/5 | Added requiresAnyFlag and hidesIfFlag properties for flexible navigation visibility logic - well-structured implementation |
| clients/admin-ui/src/features/integrations/configure-monitor/ConfigureMonitorForm.tsx | 4/5 | Added backend config check for LLM classifier support and disabled toggle with tooltip when unavailable - one logic issue with initial value handling |
| clients/admin-ui/src/pages/data-discovery/action-center/index.tsx | 5/5 | Updated to use both webMonitor and llmClassifier flags for monitor filtering and page access - logic correctly implements OR condition |
| clients/admin-ui/cypress/e2e/integration-management.cy.ts | 5/5 | Removed prompt template tests and added backend config mocking for LLM classifier - comprehensive test coverage for new server-side flag |
11 files reviewed, 1 comment
Ticket ENG-1801
Description Of Changes
Replaced the alpha
alphaFullActionCenterfeature flag with a beta-appropriatellmClassifierflag to control LLM classifier functionality and the updated Action Center UI. The Action Center now appears when eitherwebMonitorORllmClassifierflags are enabled, while the legacy Activity, Detection, and Discovery pages are hidden whenllmClassifieris enabled. Added backend configuration check (detection_discovery.llm_classifier_enabled) to disable the LLM classifier toggle with a helpful tooltip when the backend doesn't support it.Also removes the Prompt Template from the LLM options in the Monitor form as per https://ethyca.atlassian.net/browse/ENG-1774
Code Changes
alphaFullActionCenterflag tollmClassifierinflags.jsonrequiresAnyFlagandhidesIfFlagproperties to navigation config for flexible route visibility logicwebMonitorORllmClassifieris enabledllmClassifieris enableddetection_discovery.llm_classifier_enabledinConfigureMonitorFormwebMonitoris off butllmClassifieris onSteps to Confirm
detection_discovery.llm_classifier_enabledis falsellmClassifieris on butwebMonitoris offwebMonitoris enabled)Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works