-
Notifications
You must be signed in to change notification settings - Fork 84
Add label prop to feature flags #6889
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. 2 Skipped Deployments
|
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
Refactored feature flag labels from dynamic generation to explicit configuration by adding a label property to each flag definition in flags.json and updating FlagControl.tsx to read from the configuration instead of calling camelToSentenceCase().
- All 12 feature flags now have explicit labels that match the previous
camelToSentenceCase()output - Removed dependency on the
camelToSentenceCaseutility function fromFlagControl.tsx - Type definition updated to include optional
labelproperty inFlagEnvs
Issue found:
- The
labelproperty is defined as optional (label?: string) but is accessed without null checking inFlagControl.tsx:61, which could cause runtime errors if a flag doesn't have a label defined
Confidence Score: 4/5
- This PR is mostly safe to merge but has a type safety issue that should be addressed
- The refactoring is straightforward and all existing labels are properly defined. However, the
labelproperty is marked as optional in the type definition but accessed without null checks in the component. While all current flags have labels defined, this creates a type safety gap that could cause runtime errors if a future flag is added without a label. Makinglabelrequired in the type definition would prevent this issue. - Pay attention to
clients/admin-ui/src/features/common/features/types.ts- the optionallabelproperty should be made required
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| clients/admin-ui/src/flags.json | 5/5 | Added explicit label property to all 12 feature flags with matching camelToSentenceCase values |
| clients/admin-ui/src/features/common/features/types.ts | 4/5 | Added optional label?: string property to FlagEnvs type definition |
| clients/admin-ui/src/features/common/features/FlagControl.tsx | 4/5 | Updated to use FLAG_CONFIG[flag].label instead of camelToSentenceCase(), removed unused import |
4 files reviewed, 1 comment
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
Refactored feature flag configuration to use explicit label properties in flags.json instead of auto-generating labels from flag keys using camelToSentenceCase(). This provides flexibility for future customization while maintaining the same user-facing labels.
- Added
labelproperty to all 12 flag definitions inflags.jsonwith values matching previous auto-generated output - Updated
FlagEnvstype to include optionallabelproperty - Modified
FlagControlto use explicit label from configuration - Removed unused
camelToSentenceCaseimport from FlagControl
The main issue is that label is defined as optional in the type definition but accessed without null checking in FlagControl.tsx:61, which was already flagged in previous comments.
Confidence Score: 4/5
- This PR is safe to merge with low risk once the type safety issue is addressed
- The changes are straightforward and mechanical - adding explicit labels to feature flags instead of generating them dynamically. All existing labels match the previous auto-generated values, so there's no behavior change. The only concern is the type safety issue where
labelis optional but accessed without null checking, which was already identified in previous comments. This won't cause runtime issues currently since all flags have labels, but it creates technical debt. - The
types.tsfile needs attention to make thelabelproperty required instead of optional to match actual usage
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| clients/admin-ui/src/features/common/features/types.ts | 3/5 | Added optional label property to FlagEnvs type but it's accessed without null check in FlagControl |
| clients/admin-ui/src/features/common/features/FlagControl.tsx | 4/5 | Removed camelToSentenceCase import and now uses explicit label from FLAG_CONFIG |
| clients/admin-ui/src/flags.json | 5/5 | Added explicit label property to all flag definitions matching previous auto-generated values |
No files reviewed, no comments
f8f70d1 to
8ad3101
Compare
Pull Request is not mergeable
bd802d8 to
a666868
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
Decouples feature flag display labels from flag keys by adding explicit label property to flag configuration. Previously, labels were auto-generated from camelCase keys using camelToSentenceCase(). Now all 12 flags have explicit labels defined in flags.json, maintaining identical display text while enabling future customization.
- Added optional
labelfield toFlagEnvstype definition - Updated
FlagControlcomponent to readlabelfrom config withcamelToSentenceCase()fallback - Added explicit labels to all feature flags in
flags.jsonmatching previous auto-generated values - Updated documentation examples across type definitions and config
Confidence Score: 5/5
- This PR is safe to merge with minimal risk
- This is a straightforward refactoring that explicitly defines labels while maintaining backward compatibility with a fallback. All flags have labels defined, no breaking changes, and the implementation is simple and well-tested by the existing UI
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| clients/admin-ui/src/features/common/features/types.ts | 5/5 | Added optional label property to FlagEnvs type with updated documentation examples |
| clients/admin-ui/src/features/common/features/FlagControl.tsx | 5/5 | Updated to use explicit label from config with fallback to camelToSentenceCase, import path simplified |
| clients/admin-ui/src/flags.json | 5/5 | Added explicit label property to all 12 feature flags with sentence-case display names |
5 files reviewed, no comments
760cd4d to
d666467
Compare
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/two-factor-authentication |
d666467 to
e42a460
Compare
Description Of Changes
Added an explicit
labelproperty to the feature flags configuration inflags.jsonto decouple the display label from the flag key name. Previously, labels were automatically generated from flag keys usingcamelToSentenceCase(). Now labels are explicitly defined in the configuration, providing flexibility for future customization while maintaining the same user-facing labels.Code Changes
labelproperty to all flag definitions inflags.jsonwith auto-generated values matching previouscamelToSentenceCase()outputFlagEnvstype definition to include optionallabel?: stringpropertyFlagControlcomponent to useFLAG_CONFIG[flag].labelinstead of computing label from flag keycamelToSentenceCaseimport fromFlagControl.tsxSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works