From 6de0ebf09f56896b2a5b2a69eb7cac8e7ada5333 Mon Sep 17 00:00:00 2001 From: Isaac Hellendag <2823852+hellendag@users.noreply.github.com> Date: Wed, 2 Oct 2024 09:53:39 -0500 Subject: [PATCH] [ui] Flip navigation feature flag (#24985) Enable the new navigation UI by flipping the feature flag to be a "revert to legacy nav" flag, turned off by default. - Replace all `flagSettingsPage` with `flagLegacyNav`, flip the logic at each callsite. - Add a `trackEvent` call to start observing feature flag change events (Plus only) Load app. Verify that the app has the updated nav without my changing any flags. Verify behavior of new flag to revert to old nav. - [ ] `NEW` [ui] The updated navigation is now enabled for all users. You can revert to the legacy navigation via a feature flag. See [GitHub discussion](https://github.com/dagster-io/dagster/discussions/21370) for more. --- .../dagster-ui/packages/ui-core/src/app/App.tsx | 6 +++--- .../ui-core/src/app/AppTopNav/AppTopNavLinks.tsx | 2 +- .../packages/ui-core/src/app/FeatureFlags.oss.tsx | 2 +- .../app/UserSettingsDialog/UserSettingsDialog.tsx | 11 ++++++++++- .../ui-core/src/app/__tests__/AppTopNav.test.tsx | 2 +- .../src/app/useVisibleFeatureFlagRows.oss.tsx | 6 +++--- .../auto-materialization/AutomaterializationRoot.tsx | 4 ++-- .../packages/ui-core/src/overview/OverviewRoot.tsx | 12 +++++------- .../packages/ui-core/src/overview/OverviewTabs.tsx | 10 +++++----- .../ui-core/src/settings/SettingsMainPane.tsx | 4 ++-- 10 files changed, 33 insertions(+), 26 deletions(-) diff --git a/js_modules/dagster-ui/packages/ui-core/src/app/App.tsx b/js_modules/dagster-ui/packages/ui-core/src/app/App.tsx index 5d889c41828e7..8556a1ac1d69c 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/app/App.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/app/App.tsx @@ -18,7 +18,7 @@ export const App = ({banner, children}: Props) => { const {nav} = React.useContext(LayoutContext); // todo dish: Remove flag and alert once this change has shipped. - const {flagSettingsPage} = useFeatureFlags(); + const {flagLegacyNav} = useFeatureFlags(); const [didDismissNavAlert, setDidDismissNavAlert] = useStateWithStorage( 'new_navigation_alert', (json) => !!json, @@ -33,7 +33,7 @@ export const App = ({banner, children}: Props) => {
{banner}
- {flagSettingsPage && !didDismissNavAlert ? ( + {!flagLegacyNav && !didDismissNavAlert ? ( ) : null} {children} @@ -52,7 +52,7 @@ const ExperimentalNavAlert = (props: AlertProps) => { const revertToLegacyNavigation = () => { const copy = new Set(flags); - copy.delete(FeatureFlag.flagSettingsPage); + copy.add(FeatureFlag.flagLegacyNav); setFeatureFlags(Array.from(copy)); setDidDismissNavAlert(true); window.location.reload(); diff --git a/js_modules/dagster-ui/packages/ui-core/src/app/AppTopNav/AppTopNavLinks.tsx b/js_modules/dagster-ui/packages/ui-core/src/app/AppTopNav/AppTopNavLinks.tsx index 0e23731da7d8c..40948f8ec29cc 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/app/AppTopNav/AppTopNavLinks.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/app/AppTopNav/AppTopNavLinks.tsx @@ -94,7 +94,7 @@ export const navLinks = (config: Config): AppNavLinkType[] => { ), }; - if (featureEnabled(FeatureFlag.flagSettingsPage)) { + if (!featureEnabled(FeatureFlag.flagLegacyNav)) { const jobs = jobState === 'has-jobs' ? { diff --git a/js_modules/dagster-ui/packages/ui-core/src/app/FeatureFlags.oss.tsx b/js_modules/dagster-ui/packages/ui-core/src/app/FeatureFlags.oss.tsx index 04490cd562517..acc1c16f45282 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/app/FeatureFlags.oss.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/app/FeatureFlags.oss.tsx @@ -3,7 +3,7 @@ export enum FeatureFlag { flagDisableWebsockets = 'flagDisableWebsockets', flagSidebarResources = 'flagSidebarResources', flagDisableAutoLoadDefaults = 'flagDisableAutoLoadDefaults', - flagSettingsPage = 'flagSettingsPage', + flagLegacyNav = 'flagLegacyNav', flagCodeLocationPage = 'flagCodeLocationPage', flagRunsFeed = 'flagRunsFeed', } diff --git a/js_modules/dagster-ui/packages/ui-core/src/app/UserSettingsDialog/UserSettingsDialog.tsx b/js_modules/dagster-ui/packages/ui-core/src/app/UserSettingsDialog/UserSettingsDialog.tsx index 48fe170cbe02a..e75e8977328d4 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/app/UserSettingsDialog/UserSettingsDialog.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/app/UserSettingsDialog/UserSettingsDialog.tsx @@ -18,6 +18,7 @@ import {UserPreferences} from 'shared/app/UserSettingsDialog/UserPreferences.oss import {CodeLinkProtocolSelect} from '../../code-links/CodeLinkProtocol'; import {showCustomAlert} from '../CustomAlertProvider'; import {getFeatureFlags, setFeatureFlags} from '../Flags'; +import {useTrackEvent} from '../analytics'; type OnCloseFn = (event: React.SyntheticEvent) => void; type VisibleFlag = {key: string; label?: React.ReactNode; flagType: FeatureFlag}; @@ -51,6 +52,7 @@ interface DialogContentProps { * we want to render it. */ const UserSettingsDialogContent = ({onClose, visibleFlags}: DialogContentProps) => { + const trackEvent = useTrackEvent(); const [flags, setFlags] = React.useState(() => getFeatureFlags()); const [reloading, setReloading] = React.useState(false); @@ -61,7 +63,14 @@ const UserSettingsDialogContent = ({onClose, visibleFlags}: DialogContentProps) }); const toggleFlag = (flag: FeatureFlag) => { - setFlags(flags.includes(flag) ? flags.filter((f) => f !== flag) : [...flags, flag]); + const flagSet = new Set(flags); + trackEvent('feature-flag', {flag, enabled: !flagSet.has(flag)}); + if (flagSet.has(flag)) { + flagSet.delete(flag); + } else { + flagSet.add(flag); + } + setFlags(Array.from(flagSet)); }; const [arePreferencesChanged, setAreaPreferencesChanged] = React.useState(false); diff --git a/js_modules/dagster-ui/packages/ui-core/src/app/__tests__/AppTopNav.test.tsx b/js_modules/dagster-ui/packages/ui-core/src/app/__tests__/AppTopNav.test.tsx index 45b0e8de35bb1..4b91ae8a5ffbb 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/app/__tests__/AppTopNav.test.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/app/__tests__/AppTopNav.test.tsx @@ -28,6 +28,6 @@ describe('AppTopNav', () => { expect(screen.getByText('Overview').closest('a')).toHaveAttribute('href', '/overview'); expect(screen.getByText('Runs').closest('a')).toHaveAttribute('href', '/runs'); expect(screen.getByText('Assets').closest('a')).toHaveAttribute('href', '/assets'); - expect(screen.getByText('Deployment').closest('a')).toHaveAttribute('href', '/locations'); + expect(screen.getByText('Deployment').closest('a')).toHaveAttribute('href', '/deployment'); }); }); diff --git a/js_modules/dagster-ui/packages/ui-core/src/app/useVisibleFeatureFlagRows.oss.tsx b/js_modules/dagster-ui/packages/ui-core/src/app/useVisibleFeatureFlagRows.oss.tsx index 0bc200cbb4b3c..c2c5f586ddb13 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/app/useVisibleFeatureFlagRows.oss.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/app/useVisibleFeatureFlagRows.oss.tsx @@ -21,10 +21,10 @@ export const useVisibleFeatureFlagRows = () => [ flagType: FeatureFlag.flagDebugConsoleLogging, }, { - key: 'New navigation', + key: 'Revert to legacy navigation', label: ( <> - Experimental navigation ( + Revert to legacy navigation ( [ ) ), - flagType: FeatureFlag.flagSettingsPage, + flagType: FeatureFlag.flagLegacyNav, }, { key: 'New code location page', diff --git a/js_modules/dagster-ui/packages/ui-core/src/assets/auto-materialization/AutomaterializationRoot.tsx b/js_modules/dagster-ui/packages/ui-core/src/assets/auto-materialization/AutomaterializationRoot.tsx index 29dc01f97f798..65c26b3db31cf 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/assets/auto-materialization/AutomaterializationRoot.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/assets/auto-materialization/AutomaterializationRoot.tsx @@ -14,14 +14,14 @@ import {useAutoMaterializeSensorFlag} from '../AutoMaterializeSensorFlag'; // depending on their nav flag state. export const AutomaterializationRoot = () => { const automaterializeSensorsFlagState = useAutoMaterializeSensorFlag(); - const {flagSettingsPage} = useFeatureFlags(); + const {flagLegacyNav} = useFeatureFlags(); switch (automaterializeSensorsFlagState) { case 'unknown': return
; // Waiting for result case 'has-global-amp': return ; case 'has-sensor-amp': - return ; + return ; default: assertUnreachable(automaterializeSensorsFlagState); } diff --git a/js_modules/dagster-ui/packages/ui-core/src/overview/OverviewRoot.tsx b/js_modules/dagster-ui/packages/ui-core/src/overview/OverviewRoot.tsx index 77986ecda814c..27cfa872c486a 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/overview/OverviewRoot.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/overview/OverviewRoot.tsx @@ -13,7 +13,7 @@ import {InstanceBackfillsRoot} from '../instance/InstanceBackfillsRoot'; import {BackfillPage} from '../instance/backfill/BackfillPage'; export const OverviewRoot = () => { - const {flagSettingsPage} = useFeatureFlags(); + const {flagLegacyNav} = useFeatureFlags(); const automaterializeSensorsFlagState = useAutoMaterializeSensorFlag(); return ( @@ -22,22 +22,20 @@ export const OverviewRoot = () => { (flagSettingsPage ? : )} + render={() => (flagLegacyNav ? : )} /> - flagSettingsPage ? : - } + render={() => (flagLegacyNav ? : )} /> (flagSettingsPage ? : )} + render={() => (flagLegacyNav ? : )} /> - flagSettingsPage && automaterializeSensorsFlagState !== 'has-global-amp' ? ( + !flagLegacyNav && automaterializeSensorsFlagState !== 'has-global-amp' ? ( ) : ( diff --git a/js_modules/dagster-ui/packages/ui-core/src/overview/OverviewTabs.tsx b/js_modules/dagster-ui/packages/ui-core/src/overview/OverviewTabs.tsx index f6f4cd6530c6b..817f1d0f81db1 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/overview/OverviewTabs.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/overview/OverviewTabs.tsx @@ -18,7 +18,7 @@ interface Props { export const OverviewTabs = >(props: Props) => { const {refreshState, tab} = props; - const {flagSettingsPage} = useFeatureFlags(); + const {flagLegacyNav} = useFeatureFlags(); const automaterialize = useAutomaterializeDaemonStatus(); const automaterializeSensorsFlagState = useAutoMaterializeSensorFlag(); @@ -32,11 +32,11 @@ export const OverviewTabs = >(props: Props )} {/* These are flagged individually because the links must be children of `Tabs`: */} - {flagSettingsPage ? null : } - {flagSettingsPage ? null : ( + {flagLegacyNav ? : null} + {flagLegacyNav ? ( - )} - {flagSettingsPage ? null : } + ) : null} + {flagLegacyNav ? : null} {automaterializeSensorsFlagState === 'has-global-amp' ? ( { - const {flagSettingsPage} = useFeatureFlags(); - if (!flagSettingsPage) { + const {flagLegacyNav} = useFeatureFlags(); + if (flagLegacyNav) { return ; }