-
Notifications
You must be signed in to change notification settings - Fork 97
integrate feature flag and async settings #9417
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
iterion
left a 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.
I don't fully understand the react bits but as described this makes sense to me!
|
THE END OF IS_STAGING_OR_DEBUG FOR FEATURES IS NOW |
| hideOnLevel: 'project', | ||
| // Don't show in prod, consider switching to use AdamS's endpoint https://github.com/KittyCAD/common/pull/1704 | ||
| hideOnPlatform: IS_STAGING_OR_DEBUG ? undefined : 'both', | ||
| hideOnPlatform: hideWithoutFeatureFlag('new_sketch_mode', 'both'), |
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.
Hopefully this is all the diff we'll need for future feature flag settings.
| * Otherwise, it will be hidden. | ||
| */ | ||
| useNewSketchMode: new Setting<boolean>({ | ||
| hideOnLevel: 'project', |
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.
Oh shoot I think this is a copy paste from when I first added it, should this line be deleted?
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.
Do we actually want to let people (us) turn it on only in some projects? Could make sense I guess?
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.
Oh right, I really don't know. I definitely thought of it as a global thing, but maybe that's useful 🤷
src/lib/settings/initialSettings.tsx
Outdated
| * @returns An async function that resolves to the hideOnPlatform value | ||
| */ | ||
| function hideWithoutFeatureFlag( | ||
| featureFlagId: 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.
Unfortunately the feature flag value itself is not part of the openAPI spec, so this is just a string that has to match the server's 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.
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.
Update update:
I changed this line to:
featureFlagId: UserFeatureEntry['id'],
Which still resolves to a string, but maybe will update to a union type if the spec updates.
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.
Update update update:
Adam is already onto it.
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.
Very excited for this. Just a few questions and notes! Until I get to test locally, might have to be a tomorrow thing
src/lib/settings/initialSettings.tsx
Outdated
| // Try to get a token - check env first, then cookie for web | ||
| const envToken = env().VITE_ZOO_API_TOKEN | ||
| const cookieToken = getCookie() | ||
| const token = envToken || cookieToken || '' |
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.
Surprised to see this logic re-implemented here? Should we abstract out what's in useToken or whatever that hook is called?
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.
Or is it because we need to make the call much earlier than when the auth machine gets it?
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.
Oh I might be doing something wrong, though useToken is a hook right? I'm not sure if this createSettings is part of teh main react flow, looks like it gets called in singletons?
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.
Yeah I agree @pierremtb that we can extract out that shared logic out of the useToken hook to a function that can be used useToken and in non-React contexts.
Since this function returns the defaultHide value if the token is not available, I think we could alternatively weave token through as an optional parameter into this function to move this client and token setup logic from here, and instead weave the token (value if it's available) through Router, routeLoaders, loadAndValidateSettings, and resolveAsyncHideOnPlatform, so we don't need to set up the client and get the token twice. But I don't think that needs to be done here, this is great.
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.
How does fb5ab4f look?
| * Otherwise, it will be hidden. | ||
| */ | ||
| useNewSketchMode: new Setting<boolean>({ | ||
| hideOnLevel: 'project', |
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.
Do we actually want to let people (us) turn it on only in some projects? Could make sense I guess?
| hideOnPlatform?: 'web' | 'desktop' | 'both' | ||
| hideOnPlatform?: | ||
| | HideOnPlatformValue | ||
| | (() => Promise<HideOnPlatformValue | null>) |
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.
Rando question for my own curiosity: why null v. undefined here for the promise return value?
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.
Oh no reason. I never know how to decide between falsy values, I can change it if you like?
Co-authored-by: Pierre Jacquier <[email protected]>
|
e2e tests look related, having a look now. |
pierremtb
left a 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.
Works great here, tested locally in the electron app and was able to see the setting based on where or not I had Force On enabled in the dev dashboard! This is huge
|
Only thing is I pinged @franknoirot on Slack, wondering if he has a good idea of where the api call should live since he's been deep in dependency injection in the last few weeks. Not too concerned because this is only impacting an internal setting but would be best to set it up nicely as we'll quickly be building on it. |

To verify this works, you can turn it on for yourself in dev with the admin dashboard go to
https://admin-dashboard-dev.hawk-dinosaur.ts.net/app/user?id=<YOUR_NAME>@zoo.dev
replace YOUR_NAME ofc.
Async Support for Settings hideOnPlatform with Feature Flag Integration (
new_sketch_modebeing the first one)This PR adds support for async functions in the
hideOnPlatformsetting property, which lets us dynamically show or hide settings based on feature flags fetched from the API. The main use case is controlling the visibility ofuseNewSketchModebased on whether the user has thenew_sketch_modefeature flag enabled. Because looking for feature flags is probably going to be the main usecase for the async setting, there's a functionhideWithoutFeatureFlag('featureFlagName', 'both')The implementation resolves async
hideOnPlatformfunctions once during settings loading inloadAndValidateSettings(), then replaces the function with the resolved value. This makes everything synchronous after that point - all visibility checks, command bar registration, and React components work with sync values.When settings are loaded via
loadAndValidateSettings(), we callresolveAsyncHideOnPlatform()which collects all settings with asynchideOnPlatformfunctions, resolves them in parallel, and replaces the function with the resolved value directly on the setting object. Since the settings object is the same reference used everywhere (including in the actor context), this single update makes everything work synchronously afterward.The resolution happens once at startup, so all subsequent code (command bar registration, React components, etc.) can use sync helper functions without any async complexity.
Resolves #9277
CC @iterion