[Left nav refactor][5/6] Update workflow switch style#20698
[Left nav refactor][5/6] Update workflow switch style#20698daniellok-db wants to merge 4 commits intomlflow:masterfrom
Conversation
59589c9 to
0e76919
Compare
🛠 DevTools 🛠
Install mlflow from this PRFor Databricks, use the following command: |
|
@daniellok-db Thank you for the contribution! Could you fix the following issue(s)? ⚠ DCO checkThe DCO check failed. Please sign off your commit(s) by following the instructions here. See https://github.com/mlflow/mlflow/blob/master/CONTRIBUTING.md#sign-your-work for more details. |
There was a problem hiding this comment.
Pull request overview
Updates the left navigation UI to match the new workflow-based design, including moving header functionality into the sidebar and adjusting toggle/switch styling while enabling the new navigation globally.
Changes:
- Replace the header with an updated collapsible sidebar layout and new workflow switch UI.
- Move theme preference switching into the Settings page and adjust toggle labels.
- Enable workflow-based navigation globally via the feature flag.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| mlflow/server/js/src/workspaces/components/WorkspaceSelector.tsx | Adds clear behavior and layout tweaks for the workspace selector. |
| mlflow/server/js/src/settings/SettingsPage.tsx | Adds theme preference switch and improves switch labeling. |
| mlflow/server/js/src/lang/default/en.json | Updates extracted i18n strings to reflect new sidebar/settings UI copy. |
| mlflow/server/js/src/common/utils/FeatureUtils.ts | Enables workflow-based navigation by default. |
| mlflow/server/js/src/common/components/MlflowSidebarWorkflowSwitch.tsx | Introduces new pill-style workflow switch component. |
| mlflow/server/js/src/common/components/MlflowSidebarLink.tsx | Introduces reusable sidebar link component with telemetry logging. |
| mlflow/server/js/src/common/components/MlflowSidebarGatewayItems.tsx | Adds nested AI Gateway sidebar items. |
| mlflow/server/js/src/common/components/MlflowSidebarExperimentItems.tsx | Adds nested experiment-specific sidebar items. |
| mlflow/server/js/src/common/components/MlflowSidebar.tsx | Refactors sidebar to support collapsing and nested sections; adds Docs/GitHub links. |
| mlflow/server/js/src/common/components/MlflowHeader.tsx | Removes the legacy header component. |
| mlflow/server/js/src/MlflowRouter.tsx | Removes header usage and wires sidebar state into the router layout. |
Comments suppressed due to low confidence (1)
mlflow/server/js/src/MlflowRouter.tsx:66
MlflowRootRoute’s effect forcesshowSidebartotruewheneverenableWorkflowBasedNavigationis enabled (currently always true). SinceshowSidebaris now also used as the collapsed/expanded state, this will reset a user-collapsed sidebar back to expanded wheneverexperimentIdchanges (route transitions). Consider separating “visible vs collapsed” into distinct state, or gating this effect so it only runs when the legacy behavior (hide sidebar on single experiment page) is needed.
const [showSidebar, setShowSidebar] = useState(true);
const { theme } = useDesignSystemTheme();
const { experimentId } = useParams();
const enableWorkflowBasedNavigation = shouldEnableWorkflowBasedNavigation();
// Hide sidebar if we are in a single experiment page (only when feature flag is disabled)
// When feature flag is enabled, sidebar should always be visible
const isSingleExperimentPage = Boolean(experimentId);
useEffect(() => {
setShowSidebar(enableWorkflowBasedNavigation || !isSingleExperimentPage);
}, [isSingleExperimentPage, enableWorkflowBasedNavigation]);
| <div> | ||
| {isLocalServer && ( | ||
| <div | ||
| css={{ | ||
| padding: 2, | ||
| marginBottom: theme.spacing.xs, | ||
| borderRadius: theme.borders.borderRadiusMd, | ||
| background: | ||
| 'linear-gradient(90deg, rgba(232, 72, 85, 0.7), rgba(155, 93, 229, 0.7), rgba(67, 97, 238, 0.7))', | ||
| }} | ||
| > | ||
| <div | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-pressed={isPanelOpen} | ||
| css={{ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| gap: theme.spacing.sm, | ||
| paddingInline: theme.spacing.md, | ||
| paddingInline: showSidebar ? theme.spacing.md : 0, | ||
| paddingBlock: theme.spacing.xs, | ||
| borderRadius: theme.borders.borderRadiusMd - 2, | ||
| justifyContent: showSidebar ? 'flex-start' : 'center', | ||
| cursor: 'pointer', | ||
| background: theme.colors.backgroundSecondary, | ||
| color: isPanelOpen ? theme.colors.actionDefaultIconHover : theme.colors.actionDefaultIconDefault, | ||
| }} | ||
| onClick={handleAssistantToggle} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| handleAssistantToggle(); | ||
| } | ||
| }} | ||
| onMouseEnter={() => setIsAssistantHovered(true)} | ||
| onMouseLeave={() => setIsAssistantHovered(false)} | ||
| > | ||
| <AssistantSparkleIcon isHovered={isAssistantHovered} /> | ||
| <Typography.Text color="primary"> | ||
| <FormattedMessage defaultMessage="Assistant" description="Sidebar button for AI assistant" /> | ||
| </Typography.Text> | ||
| <Tag componentId="mlflow.sidebar.assistant_beta_tag" color="turquoise" css={{ marginLeft: 'auto' }}> | ||
| Beta | ||
| </Tag> | ||
| {showSidebar && ( | ||
| <> | ||
| <Typography.Text color="primary"> | ||
| <FormattedMessage defaultMessage="Assistant" description="Sidebar button for AI assistant" /> | ||
| </Typography.Text> | ||
| <Tag componentId="mlflow.sidebar.assistant_beta_tag" color="turquoise" css={{ marginLeft: 'auto' }}> | ||
| Beta | ||
| </Tag> | ||
| </> | ||
| )} | ||
| </div> | ||
| </div> | ||
| )} | ||
| <Link | ||
| <MlflowSidebarLink | ||
| disableWorkspacePrefix | ||
| css={{ paddingBlock: theme.spacing.sm }} | ||
| to={HomePageDocsUrl} | ||
| componentId="mlflow.sidebar.docs_link" | ||
| isActive={() => false} | ||
| icon={<InfoBookIcon />} | ||
| collapsed={!showSidebar} | ||
| openInNewTab | ||
| > | ||
| <FormattedMessage defaultMessage="Docs" description="Sidebar link for docs page" /> | ||
| </MlflowSidebarLink> | ||
| <MlflowSidebarLink | ||
| disableWorkspacePrefix | ||
| css={{ paddingBlock: theme.spacing.sm }} | ||
| to="https://github.com/mlflow/mlflow" | ||
| componentId="mlflow.sidebar.github_link" | ||
| isActive={() => false} | ||
| icon={<CodeIcon />} | ||
| collapsed={!showSidebar} | ||
| openInNewTab | ||
| > | ||
| <FormattedMessage defaultMessage="GitHub" description="Sidebar link for GitHub page" /> | ||
| </MlflowSidebarLink> | ||
| <MlflowSidebarLink | ||
| disableWorkspacePrefix | ||
| css={{ paddingBlock: theme.spacing.sm }} | ||
| to={ExperimentTrackingRoutes.settingsPageRoute} | ||
| aria-current={isSettingsActive(location) ? 'page' : undefined} | ||
| css={{ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| gap: theme.spacing.sm, | ||
| color: theme.colors.textPrimary, | ||
| paddingInline: theme.spacing.md, | ||
| paddingBlock: theme.spacing.sm, | ||
| borderRadius: theme.borders.borderRadiusSm, | ||
| '&:hover': { | ||
| color: theme.colors.actionLinkHover, | ||
| backgroundColor: theme.colors.actionDefaultBackgroundHover, | ||
| }, | ||
| '&[aria-current="page"]': { | ||
| backgroundColor: theme.colors.actionDefaultBackgroundPress, | ||
| color: theme.isDarkMode ? theme.colors.blue300 : theme.colors.blue700, | ||
| fontWeight: theme.typography.typographyBoldFontWeight, | ||
| }, | ||
| }} | ||
| onClick={() => | ||
| logTelemetryEvent({ | ||
| componentId: 'mlflow.sidebar.settings_tab_link', | ||
| componentViewId: viewId, | ||
| componentType: DesignSystemEventProviderComponentTypes.TypographyLink, | ||
| componentSubType: null, | ||
| eventType: DesignSystemEventProviderAnalyticsEventTypes.OnClick, | ||
| }) | ||
| } | ||
| componentId="mlflow.sidebar.settings_tab_link" | ||
| isActive={isSettingsActive} | ||
| icon={<GearIcon />} | ||
| collapsed={!showSidebar} | ||
| > | ||
| <GearIcon /> | ||
| <FormattedMessage defaultMessage="Settings" description="Sidebar link for settings page" /> | ||
| </Link> | ||
| </MlflowSidebarLink> | ||
| </div> |
There was a problem hiding this comment.
MlflowSidebarLink renders a <li> internally, but it’s used outside of a <ul>/<ol> in the bottom section (Docs/GitHub/Settings are inside a <div>). This produces invalid HTML and can harm accessibility/styling. Either render those links inside a list container, or refactor MlflowSidebarLink to return the Link only and let callers decide whether to wrap it in <li>.
| import type { Location } from '../utils/RoutingUtils'; | ||
| import { MlflowSidebarLink } from './MlflowSidebarLink'; | ||
|
|
||
| const isEndpointsActive = (location: Location) => Boolean(matchPath('/gateway', location.pathname)); |
There was a problem hiding this comment.
isEndpointsActive currently matches any /gateway... path, including /gateway/api-keys, so both “Endpoints” and “API Keys” can appear active at the same time. Restore the previous exclusion logic (treat /gateway/api-keys as not endpoints) or tighten the match (e.g., exact match for the endpoints route).
| const isEndpointsActive = (location: Location) => Boolean(matchPath('/gateway', location.pathname)); | |
| const isEndpointsActive = (location: Location) => | |
| Boolean(matchPath('/gateway', location.pathname) && !matchPath('/gateway/api-keys', location.pathname)); |
| {showWorkspaceMenuItems && | ||
| menuItems.map( | ||
| ({ key, icon, linkProps, componentId, nestedItems }) => | ||
| nestedItems ?? ( | ||
| <MlflowSidebarLink | ||
| to={linkProps.to} | ||
| aria-current={linkProps.isActive(location) ? 'page' : undefined} | ||
| css={{ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| gap: theme.spacing.sm, | ||
| color: theme.colors.textPrimary, | ||
| padding: `${theme.spacing.xs}px ${theme.spacing.sm}px`, | ||
| borderRadius: theme.borders.borderRadiusSm, | ||
| '&:hover': { | ||
| color: theme.colors.actionLinkHover, | ||
| backgroundColor: theme.colors.actionDefaultBackgroundHover, | ||
| }, | ||
| '&[aria-current="page"]': { | ||
| backgroundColor: theme.colors.actionDefaultBackgroundPress, | ||
| color: theme.isDarkMode ? theme.colors.blue300 : theme.colors.blue700, | ||
| fontWeight: theme.typography.typographyBoldFontWeight, | ||
| }, | ||
| }} | ||
| onClick={() => | ||
| logTelemetryEvent({ | ||
| componentId, | ||
| componentViewId: viewId, | ||
| componentType: DesignSystemEventProviderComponentTypes.TypographyLink, | ||
| componentSubType: null, | ||
| eventType: DesignSystemEventProviderAnalyticsEventTypes.OnClick, | ||
| }) | ||
| } | ||
| componentId={componentId} | ||
| isActive={linkProps.isActive} | ||
| icon={icon} | ||
| collapsed={!showSidebar} | ||
| > | ||
| {icon} | ||
| {linkProps.children} | ||
| </Link> | ||
| {nestedItemsGroups && nestedItemsGroups.length > 0 && ( | ||
| <ul css={NESTED_ITEMS_UL_CSS}> | ||
| {nestedItemsGroups.map((group) => ( | ||
| <Fragment key={group.sectionKey}> | ||
| {group.sectionKey !== 'top-level' && ( | ||
| <li | ||
| css={{ | ||
| display: 'flex', | ||
| marginTop: theme.spacing.xs, | ||
| marginBottom: theme.spacing.xs, | ||
| position: 'relative', | ||
| height: theme.typography.lineHeightBase, | ||
| paddingLeft: 40, | ||
| }} | ||
| > | ||
| <Typography.Text size="sm" color="secondary"> | ||
| {getExperimentPageSideNavSectionLabel(group.sectionKey, [])} | ||
| </Typography.Text> | ||
| </li> | ||
| )} | ||
| {group.items.map((nestedItem) => { | ||
| const isDisabled = !experimentId && key === 'experiments'; | ||
| return <li key={nestedItem.key}>{renderNestedItemLink(nestedItem, isDisabled)}</li>; | ||
| })} | ||
| </Fragment> | ||
| ))} | ||
| </ul> | ||
| )} | ||
| {nestedItems && nestedItems.length > 0 && ( | ||
| <ul css={NESTED_ITEMS_UL_CSS}> | ||
| {nestedItems.map((nestedItem) => ( | ||
| <li key={nestedItem.key}>{renderNestedItemLink(nestedItem, false)}</li> | ||
| ))} | ||
| </ul> | ||
| )} | ||
| </li> | ||
| ))} | ||
| </> | ||
| )} | ||
| </MlflowSidebarLink> | ||
| ), | ||
| )} |
There was a problem hiding this comment.
menuItems.map(...) does not provide a key prop for the returned elements. The key on the <li> inside MlflowSidebarLink does not satisfy React’s requirement (keys must be on the immediate children of the array). Add key={key} (or similar stable key) on the returned element/fragment for both the nestedItems and the default link case.
| SegmentedControlGroup, | ||
| SegmentedControlButton, | ||
| Tag, |
There was a problem hiding this comment.
SegmentedControlGroup and SegmentedControlButton are imported but no longer used after switching to MlflowSidebarWorkflowSwitch. With noUnusedLocals/lint rules this will fail CI; remove the unused imports.
| SegmentedControlGroup, | |
| SegmentedControlButton, | |
| Tag, |
| return ( | ||
| <div | ||
| css={{ | ||
| display: 'flex', | ||
| flex: 1, | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| cursor: 'pointer', | ||
| // magic number to prevent the text from jumping around | ||
| // when switching between workflow types | ||
| paddingRight: workflowType === WorkflowType.GENAI ? 0 : 7, | ||
| paddingLeft: workflowType === WorkflowType.GENAI ? 7 : 0, | ||
| }} | ||
| onClick={() => setWorkflowType(workflowType)} | ||
| > | ||
| <Typography.Text>{label}</Typography.Text> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Pill uses clickable <div> elements with onClick, but there’s no keyboard interaction, focus styling, or ARIA semantics (it replaced the previous SegmentedControlGroup which handled accessibility). Consider using actual <button> elements or a DS segmented control, or add role="radiogroup"/role="radio", tabIndex, and onKeyDown handlers so the workflow switch is keyboard-accessible.
| export const MlflowSidebarLink = ({ | ||
| className, | ||
| to, | ||
| componentId, | ||
| icon, | ||
| children, | ||
| isActive, | ||
| onClick, | ||
| openInNewTab = false, | ||
| collapsed = false, | ||
| disableWorkspacePrefix = false, | ||
| }: { | ||
| className?: string; | ||
| to: string; | ||
| componentId: string; | ||
| icon?: React.ReactNode; | ||
| children: React.ReactNode; | ||
| isActive: (location: Location) => boolean; | ||
| onClick?: () => void; | ||
| openInNewTab?: boolean; | ||
| collapsed?: boolean; | ||
| disableWorkspacePrefix?: boolean; | ||
| }) => { |
There was a problem hiding this comment.
MlflowSidebarLink is used with a css prop in multiple callers (e.g. MlflowSidebarGatewayItems, MlflowSidebarExperimentItems, MlflowSidebar), but this component’s props don’t declare/forward css. This will fail TypeScript type-checking and also prevents callers from applying the intended spacing. Add an explicit css (or style) prop and merge it into the underlying Link styles, or switch callers to className only.
| }: { | ||
| className?: string; | ||
| to: string; | ||
| componentId: string; | ||
| icon?: React.ReactNode; | ||
| children: React.ReactNode; | ||
| isActive: (location: Location) => boolean; | ||
| onClick?: () => void; | ||
| openInNewTab?: boolean; | ||
| collapsed?: boolean; | ||
| disableWorkspacePrefix?: boolean; |
There was a problem hiding this comment.
This file references React.ReactNode in the prop types but doesn’t import React (or ReactNode) anywhere. In TS this typically produces Cannot find name 'React' when noImplicitAny/types aren’t providing a global React namespace. Prefer import type { ReactNode } from 'react' and use ReactNode in the props, or add a type-only React import.
| {Object.entries(config).map(([sectionKey, items]) => ( | ||
| <> | ||
| {sectionKey !== 'top-level' && collapsed ? ( |
There was a problem hiding this comment.
Object.entries(config).map(...) returns a fragment (<>...</>) without a key, which will produce React warnings and can lead to incorrect reconciliation when the config changes. Wrap the block in React.Fragment key={sectionKey} (or equivalent) so each section is keyed.
| <> | ||
| <div | ||
| css={{ | ||
| borderTop: `1px solid ${theme.colors.actionDefaultBorderDefault}`, | ||
| width: '100%', | ||
| paddingBottom: theme.spacing.sm, | ||
| marginTop: theme.spacing.xs, | ||
| }} | ||
| /> | ||
| <MlflowSidebarLink |
There was a problem hiding this comment.
This component renders several <div> separators (e.g. the border lines at the top/bottom and collapsed section separators). When MlflowSidebarExperimentItems is injected directly into the parent <ul>, those <div> children create invalid list structure (<ul> should only contain <li>). Consider converting separators to <li> elements (with role="separator" if needed) or have the parent render this content outside the <ul>.
|
Documentation preview for 3b1a6a8 is available at: More info
|
0e76919 to
4ac43ec
Compare
serena-ruan
left a comment
There was a problem hiding this comment.
LGTM! I like the new style :D
4ac43ec to
93e75c3
Compare
2e85d86 to
d0224ed
Compare
|
closing this as i'm just merging the top PR of the stack |
Pull request was closed
🥞 Stacked PR
Use this link to review incremental changes.
Related Issues/PRs
#xxxWhat changes are proposed in this pull request?
This PR updates the workflow toggle to match the design spec. It also removes the redundant "Workflow type" header/
This PR also flips the flag to true to enable this new nav globally!
How is this PR tested?
Looks like this
Screen.Recording.2026-02-11.at.1.56.36.PM.mov
Does this PR require documentation update?
Does this PR require updating the MLflow Skills repository?
Release Notes
Is this a user-facing change?
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/tracking: Tracking Service, tracking client APIs, autologgingarea/models: MLmodel format, model serialization/deserialization, flavorsarea/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registryarea/scoring: MLflow Model server, model deployment tools, Spark UDFsarea/evaluation: MLflow model evaluation features, evaluation metrics, and evaluation workflowsarea/gateway: MLflow AI Gateway client APIs, server, and third-party integrationsarea/prompts: MLflow prompt engineering features, prompt templates, and prompt managementarea/tracing: MLflow Tracing features, tracing APIs, and LLM tracing functionalityarea/projects: MLproject format, project running backendsarea/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev serverarea/build: Build and test infrastructure for MLflowarea/docs: MLflow documentation pagesHow should the PR be classified in the release notes? Choose one:
rn/none- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/breaking-change- The PR will be mentioned in the "Breaking Changes" sectionrn/feature- A new user-facing feature worth mentioning in the release notesrn/bug-fix- A user-facing bug fix worth mentioning in the release notesrn/documentation- A user-facing documentation change worth mentioning in the release notesShould this PR be included in the next patch release?
Yesshould be selected for bug fixes, documentation updates, and other small changes.Noshould be selected for new features and larger changes. If you're unsure about the release classification of this PR, leave this unchecked to let the maintainers decide.What is a minor/patch release?
Bug fixes, doc updates and new features usually go into minor releases.
Bug fixes and doc updates usually go into patch releases.