Skip to content

Commit

Permalink
[ui] Flip navigation feature flag (#24985)
Browse files Browse the repository at this point in the history
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](#21370) for
more.
  • Loading branch information
hellendag committed Oct 3, 2024
1 parent 0f59b0a commit 6de0ebf
Show file tree
Hide file tree
Showing 10 changed files with 33 additions and 26 deletions.
6 changes: 3 additions & 3 deletions js_modules/dagster-ui/packages/ui-core/src/app/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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<boolean>(
'new_navigation_alert',
(json) => !!json,
Expand All @@ -33,7 +33,7 @@ export const App = ({banner, children}: Props) => {
<LeftNav />
<Main $navOpen={nav.isOpen} onClick={onClickMain}>
<div>{banner}</div>
{flagSettingsPage && !didDismissNavAlert ? (
{!flagLegacyNav && !didDismissNavAlert ? (
<ExperimentalNavAlert setDidDismissNavAlert={setDidDismissNavAlert} />
) : null}
<ChildContainer>{children}</ChildContainer>
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export const navLinks = (config: Config): AppNavLinkType[] => {
),
};

if (featureEnabled(FeatureFlag.flagSettingsPage)) {
if (!featureEnabled(FeatureFlag.flagLegacyNav)) {
const jobs =
jobState === 'has-jobs'
? {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ export enum FeatureFlag {
flagDisableWebsockets = 'flagDisableWebsockets',
flagSidebarResources = 'flagSidebarResources',
flagDisableAutoLoadDefaults = 'flagDisableAutoLoadDefaults',
flagSettingsPage = 'flagSettingsPage',
flagLegacyNav = 'flagLegacyNav',
flagCodeLocationPage = 'flagCodeLocationPage',
flagRunsFeed = 'flagRunsFeed',
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<HTMLElement>) => void;
type VisibleFlag = {key: string; label?: React.ReactNode; flagType: FeatureFlag};
Expand Down Expand Up @@ -51,6 +52,7 @@ interface DialogContentProps {
* we want to render it.
*/
const UserSettingsDialogContent = ({onClose, visibleFlags}: DialogContentProps) => {
const trackEvent = useTrackEvent();
const [flags, setFlags] = React.useState<FeatureFlag[]>(() => getFeatureFlags());
const [reloading, setReloading] = React.useState(false);

Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<a
href="https://github.com/dagster-io/dagster/discussions/21370"
target="_blank"
Expand All @@ -35,7 +35,7 @@ export const useVisibleFeatureFlagRows = () => [
)
</>
),
flagType: FeatureFlag.flagSettingsPage,
flagType: FeatureFlag.flagLegacyNav,
},
{
key: 'New code location page',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 <div />; // Waiting for result
case 'has-global-amp':
return <GlobalAutomaterializationRoot />;
case 'has-sensor-amp':
return <Redirect to={flagSettingsPage ? '/automation' : '/overview/sensors'} />;
return <Redirect to={flagLegacyNav ? '/overview/sensors' : '/automation'} />;
default:
assertUnreachable(automaterializeSensorsFlagState);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<Switch>
Expand All @@ -22,22 +22,20 @@ export const OverviewRoot = () => {
</Route>
<Route
path="/overview/jobs"
render={() => (flagSettingsPage ? <Redirect to="/jobs" /> : <OverviewJobsRoot />)}
render={() => (flagLegacyNav ? <OverviewJobsRoot /> : <Redirect to="/jobs" />)}
/>
<Route
path="/overview/schedules"
render={() =>
flagSettingsPage ? <Redirect to="/automation" /> : <OverviewSchedulesRoot />
}
render={() => (flagLegacyNav ? <OverviewSchedulesRoot /> : <Redirect to="/automation" />)}
/>
<Route
path="/overview/sensors"
render={() => (flagSettingsPage ? <Redirect to="/automation" /> : <OverviewSensorsRoot />)}
render={() => (flagLegacyNav ? <OverviewSensorsRoot /> : <Redirect to="/automation" />)}
/>
<Route
path="/overview/automation"
render={() =>
flagSettingsPage && automaterializeSensorsFlagState !== 'has-global-amp' ? (
!flagLegacyNav && automaterializeSensorsFlagState !== 'has-global-amp' ? (
<Redirect to="/automation" />
) : (
<AutomaterializationRoot />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ interface Props<TData> {
export const OverviewTabs = <TData extends Record<string, any>>(props: Props<TData>) => {
const {refreshState, tab} = props;

const {flagSettingsPage} = useFeatureFlags();
const {flagLegacyNav} = useFeatureFlags();

const automaterialize = useAutomaterializeDaemonStatus();
const automaterializeSensorsFlagState = useAutoMaterializeSensorFlag();
Expand All @@ -32,11 +32,11 @@ export const OverviewTabs = <TData extends Record<string, any>>(props: Props<TDa
<TabLink id="asset-health" title="Asset health" to="/overview/asset-health" />
)}
{/* These are flagged individually because the links must be children of `Tabs`: */}
{flagSettingsPage ? null : <TabLink id="jobs" title="Jobs" to="/overview/jobs" />}
{flagSettingsPage ? null : (
{flagLegacyNav ? <TabLink id="jobs" title="Jobs" to="/overview/jobs" /> : null}
{flagLegacyNav ? (
<TabLink id="schedules" title="Schedules" to="/overview/schedules" />
)}
{flagSettingsPage ? null : <TabLink id="sensors" title="Sensors" to="/overview/sensors" />}
) : null}
{flagLegacyNav ? <TabLink id="sensors" title="Sensors" to="/overview/sensors" /> : null}
{automaterializeSensorsFlagState === 'has-global-amp' ? (
<TabLink
id="amp"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import {InstanceConfigContent} from '../instance/InstanceConfig';
import {InstanceHealthPageContent} from '../instance/InstanceHealthPage';

export const SettingsMainPane = () => {
const {flagSettingsPage} = useFeatureFlags();
if (!flagSettingsPage) {
const {flagLegacyNav} = useFeatureFlags();
if (flagLegacyNav) {
return <Redirect to="/locations" />;
}

Expand Down

0 comments on commit 6de0ebf

Please sign in to comment.