| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
This PR introduces compile-time type safety for feature flag lookups by ensuring that CodeQL is passed when required. The key changes include:
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/feature-flags.ts | Refactored type definitions and interface to add compile-time safety for CodeQL requirements; added FeatureWithoutCLI type and method overloads |
| src/feature-flags.test.ts | Updated test helper function getFeatureIncludingCodeQlIfRequired to use new type system; added imports for new types |
| src/testing-utils.ts | Added type cast in mock getValue implementation to support both method overloads |
| lib/*.js | Auto-generated JavaScript files reflecting the TypeScript changes (not reviewed per guidelines) |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM, other than one minor point. Very similar to what I came up with :)
Sorry, something went wrong.
| }, | ||
| getValue: async (feature) => { | ||
| return enabledFeatures.includes(feature); | ||
| return enabledFeatures.includes(feature as Feature); |
There was a problem hiding this comment.
Minor: could this be a type annotation on feature?
Sorry, something went wrong.
There was a problem hiding this comment.
I agree that's slightly better, but I don't think it's worth going through another round of approval :)
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Introduce a type-level check that CodeQL is passed during feature flag lookup if it is needed.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Dev change only.
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist