Skip to content

Commit 2d76fc0

Browse files
DaoDaoNoCodeopenshift-merge-bot[bot]
DaoDaoNoCode
authored and
openshift-merge-bot[bot]
committed
Refactor PF override files and fix several UX issues
Signed-off-by: Juntao Wang <juntwang@redhat.com>
1 parent fe1c2f5 commit 2d76fc0

21 files changed

Lines changed: 2565 additions & 1883 deletions

‎mlflow/server/js/src/common/components/DesignSystemContainer.test.tsx‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,23 +37,43 @@ describe('DesignSystemContainer', () => {
3737
},
3838
);
3939

40-
test('should not attach additional container while in document.body', () => {
40+
test('should use the dedicated popup container while in document.body', () => {
4141
render(
4242
<DesignSystemContainer>
4343
<span>hello</span>
4444
</DesignSystemContainer>,
4545
);
4646
expect(screen.getByText('hello')).toBeInTheDocument();
47-
expect(mockGetPopupContainerFn()).toBe(document.body);
47+
expect(screen.getByText('hello').closest('.pf-shell-container')).toHaveClass('pf-shell-root');
48+
expect(mockGetPopupContainerFn()).not.toBe(document.body);
49+
expect(mockGetPopupContainerFn()).toHaveClass('pf-shell-root');
4850
});
4951

50-
test('should attach additional container while in shadow DOM', () => {
52+
test('should use the dedicated popup container while in shadow DOM', () => {
5153
const customElement = window.document.createElement('demo-shadow-dom');
5254
window.document.body.appendChild(customElement);
5355

5456
expect(mockGetPopupContainerFn()).not.toBe(document.body);
5557
expect(mockGetPopupContainerFn().tagName).toBe('DIV');
58+
expect(mockGetPopupContainerFn()).toHaveClass('pf-shell-root');
5659

5760
expect(1).toBe(1);
5861
});
62+
63+
test('should mirror dark mode classes onto standalone shell containers', () => {
64+
const shadowHost = window.document.createElement('div');
65+
const shadowRoot = shadowHost.attachShadow({ mode: 'open' });
66+
window.document.body.appendChild(shadowHost);
67+
68+
render(
69+
<DesignSystemContainer isDarkTheme>
70+
<span>hello in dark mode</span>
71+
</DesignSystemContainer>,
72+
{
73+
baseElement: shadowRoot,
74+
},
75+
);
76+
77+
expect(mockGetPopupContainerFn()).toHaveClass('pf-shell-root', 'dark-mode');
78+
});
5979
});

‎mlflow/server/js/src/common/components/DesignSystemContainer.tsx‎

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@ import { PATTERN_FLY_TOKEN_TRANSLATION } from '../styles/patternfly/patternflyTo
66
import { ThemeProvider as EmotionThemeProvider } from '@emotion/react';
77
import '../styles/patternfly/pf-shell-overrides.scss';
88

9-
const isInsideShadowDOM = (element: HTMLDivElement | null): boolean =>
10-
element instanceof window.Node && element.getRootNode() !== document;
9+
const PF_SHELL_CONTAINER_CLASS_NAME = 'pf-shell-container';
10+
const PF_SHELL_ROOT_CLASS_NAME = 'pf-shell-root';
11+
const DARK_MODE_CLASS_NAME = 'dark-mode';
1112

1213
type DesignSystemContainerProps = {
1314
isDarkTheme?: boolean;
@@ -24,28 +25,23 @@ export const MLflowImagePreviewContainer = React.createContext({
2425
});
2526

2627
/**
27-
* MFE-safe DesignSystemProvider that checks if the application is
28-
* in the context of the Shadow DOM and if true, provides dedicated
29-
* DOM element for the purpose of housing modals/popups there.
28+
* MFE-safe DesignSystemProvider that keeps portal content inside a dedicated
29+
* MLflow-owned shell node so PatternFly-scoped overrides apply consistently.
30+
* When mounted in a Shadow DOM, that node remains inside the same shadow root.
3031
*/
3132
export const DesignSystemContainer = (props: DesignSystemContainerProps) => {
3233
const modalContainerElement = useRef<HTMLDivElement | null>(null);
3334
const { isDarkTheme = false, children } = props;
35+
const shellRootClassName = `${PF_SHELL_ROOT_CLASS_NAME}${isDarkTheme ? ` ${DARK_MODE_CLASS_NAME}` : ''}`;
3436

35-
const getPopupContainer = useCallback(() => {
36-
const modelContainerEle = modalContainerElement.current;
37-
if (modelContainerEle !== null && isInsideShadowDOM(modelContainerEle)) {
38-
return modelContainerEle;
39-
}
40-
return document.body;
41-
}, []);
37+
const getPopupContainer = useCallback(() => modalContainerElement.current ?? document.body, []);
4238

4339
// Specialized container for antd image previews, always rendered near MLflow
4440
// to maintain prefixed CSS classes and styles.
4541
const getImagePreviewPopupContainer = useCallback(() => {
46-
const modelContainerEle = modalContainerElement.current;
47-
if (modelContainerEle !== null) {
48-
return modelContainerEle;
42+
const modalContainerEle = modalContainerElement.current;
43+
if (modalContainerEle !== null) {
44+
return modalContainerEle;
4945
}
5046
return document.body;
5147
}, []);
@@ -55,8 +51,8 @@ export const DesignSystemContainer = (props: DesignSystemContainerProps) => {
5551
<DesignSystemProvider getPopupContainer={getPopupContainer} {...props}>
5652
<MLflowImagePreviewContainer.Provider value={{ getImagePreviewPopupContainer }}>
5753
<EmotionThemeProvider theme={(baseTheme) => PATTERN_FLY_TOKEN_TRANSLATION(baseTheme)}>
58-
<div className="pf-shell-container">{children}</div>
59-
<div ref={modalContainerElement} />
54+
<div className={`${PF_SHELL_CONTAINER_CLASS_NAME} ${shellRootClassName}`}>{children}</div>
55+
<div ref={modalContainerElement} className={shellRootClassName} />
6056
</EmotionThemeProvider>
6157
</MLflowImagePreviewContainer.Provider>
6258
</DesignSystemProvider>

‎mlflow/server/js/src/common/hooks/useEditKeyValueTagsModal.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ export const useEditKeyValueTagsModal = <T extends { tags?: KeyValueEntity[] }>(
204204
css={{ display: 'flex', alignItems: 'flex-end', gap: theme.spacing.md }}
205205
>
206206
<div css={{ minWidth: 0, display: 'flex', gap: theme.spacing.md, flex: 1 }}>
207-
<div css={{ flex: 1 }}>
207+
<div css={{ flex: 1 }} data-pf-tag-key-select-error={form.formState.errors.key ? 'true' : undefined}>
208208
<FormUI.Label htmlFor="key">
209209
{intl.formatMessage({
210210
defaultMessage: 'Key',

‎mlflow/server/js/src/common/hooks/useEmbeddedLinkInterceptor.test.tsx‎

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
import { act, renderHook, waitFor } from '@testing-library/react';
2+
import { beforeEach, describe, expect, it, jest } from '@jest/globals';
3+
import { useEmbeddedLinkInterceptor } from './useEmbeddedLinkInterceptor';
4+
import { isIntegrated } from '../utils/embedUtils';
5+
6+
jest.mock('../utils/embedUtils', () => ({
7+
isIntegrated: jest.fn(),
8+
}));
9+
10+
const GUARDED_LINK_ATTR = 'data-mlflow-embedded-link-guard';
11+
12+
const createAnchor = (href: string) => {
13+
const anchor = document.createElement('a');
14+
anchor.href = href;
15+
anchor.textContent = href;
16+
return anchor;
17+
};
18+
19+
const createFederatedWrapper = () => {
20+
const wrapper = document.createElement('div');
21+
wrapper.className = 'mlflow-federated';
22+
document.body.appendChild(wrapper);
23+
return wrapper;
24+
};
25+
26+
const createFederatedPortalRoot = () => {
27+
const portalRoot = document.createElement('div');
28+
portalRoot.setAttribute('data-radix-popper-content-wrapper', '');
29+
document.body.appendChild(portalRoot);
30+
return portalRoot;
31+
};
32+
33+
const createFederatedPortalContainer = () => {
34+
const portalContainer = document.createElement('div');
35+
portalContainer.setAttribute('data-mlflow-federated-portal-container', 'true');
36+
document.body.appendChild(portalContainer);
37+
return portalContainer;
38+
};
39+
40+
const createMlflowOwnedMarker = () => {
41+
const marker = document.createElement('div');
42+
marker.setAttribute('data-component-id', 'mlflow.test.portal');
43+
return marker;
44+
};
45+
46+
describe('useEmbeddedLinkInterceptor', () => {
47+
beforeEach(() => {
48+
jest.clearAllMocks();
49+
(isIntegrated as jest.Mock).mockReturnValue(true);
50+
document.body.innerHTML = '';
51+
document.body.setAttribute('data-mlflow-federated', 'true');
52+
});
53+
54+
it('guards restricted links inside the federated wrapper on mount', async () => {
55+
const wrapper = createFederatedWrapper();
56+
const restrictedLink = createAnchor('/models/test-model');
57+
const allowedLink = createAnchor('/experiments/23/models/test-model');
58+
59+
wrapper.append(restrictedLink, allowedLink);
60+
61+
renderHook(() => useEmbeddedLinkInterceptor());
62+
63+
await waitFor(() => {
64+
expect(restrictedLink).toHaveAttribute('tabindex', '-1');
65+
});
66+
67+
expect(restrictedLink).toHaveAttribute(GUARDED_LINK_ATTR, 'true');
68+
expect(allowedLink).not.toHaveAttribute(GUARDED_LINK_ATTR);
69+
});
70+
71+
it('guards restricted links added after mount', async () => {
72+
const wrapper = createFederatedWrapper();
73+
renderHook(() => useEmbeddedLinkInterceptor());
74+
75+
const restrictedLink = createAnchor('/models/test-model');
76+
act(() => {
77+
wrapper.appendChild(restrictedLink);
78+
});
79+
80+
await waitFor(() => {
81+
expect(restrictedLink).toHaveAttribute(GUARDED_LINK_ATTR, 'true');
82+
});
83+
});
84+
85+
it('restores guarded links when their href becomes allowed', async () => {
86+
const wrapper = createFederatedWrapper();
87+
const restrictedLink = createAnchor('/models/test-model');
88+
wrapper.appendChild(restrictedLink);
89+
90+
renderHook(() => useEmbeddedLinkInterceptor());
91+
92+
await waitFor(() => {
93+
expect(restrictedLink).toHaveAttribute(GUARDED_LINK_ATTR, 'true');
94+
});
95+
96+
act(() => {
97+
restrictedLink.setAttribute('href', '/experiments/23/models/test-model');
98+
});
99+
100+
await waitFor(() => {
101+
expect(restrictedLink).not.toHaveAttribute(GUARDED_LINK_ATTR);
102+
});
103+
104+
expect(restrictedLink).not.toHaveAttribute('tabindex');
105+
});
106+
107+
it('restores guarded links when the federated wrapper loses scope', async () => {
108+
const wrapper = createFederatedWrapper();
109+
const restrictedLink = createAnchor('/models/test-model');
110+
wrapper.appendChild(restrictedLink);
111+
112+
renderHook(() => useEmbeddedLinkInterceptor());
113+
114+
await waitFor(() => {
115+
expect(restrictedLink).toHaveAttribute(GUARDED_LINK_ATTR, 'true');
116+
});
117+
118+
act(() => {
119+
wrapper.className = '';
120+
});
121+
122+
await waitFor(() => {
123+
expect(restrictedLink).not.toHaveAttribute(GUARDED_LINK_ATTR);
124+
});
125+
126+
expect(restrictedLink).not.toHaveAttribute('tabindex');
127+
});
128+
129+
it('guards restricted links inside MLflow-owned body portals', async () => {
130+
const portalRoot = createFederatedPortalRoot();
131+
const restrictedLink = createAnchor('/models/test-model');
132+
133+
portalRoot.append(createMlflowOwnedMarker(), restrictedLink);
134+
135+
renderHook(() => useEmbeddedLinkInterceptor());
136+
137+
await waitFor(() => {
138+
expect(restrictedLink).toHaveAttribute(GUARDED_LINK_ATTR, 'true');
139+
});
140+
});
141+
142+
it('guards restricted links inside the dedicated federated portal container', async () => {
143+
const portalContainer = createFederatedPortalContainer();
144+
const portalRoot = document.createElement('div');
145+
portalRoot.setAttribute('data-radix-popper-content-wrapper', '');
146+
const restrictedLink = createAnchor('/models/test-model');
147+
148+
portalRoot.appendChild(restrictedLink);
149+
portalContainer.appendChild(portalRoot);
150+
151+
renderHook(() => useEmbeddedLinkInterceptor());
152+
153+
await waitFor(() => {
154+
expect(restrictedLink).toHaveAttribute(GUARDED_LINK_ATTR, 'true');
155+
});
156+
});
157+
158+
it('ignores generic body portals until they become MLflow-owned', async () => {
159+
const portalRoot = createFederatedPortalRoot();
160+
const restrictedLink = createAnchor('/models/test-model');
161+
162+
portalRoot.appendChild(restrictedLink);
163+
164+
renderHook(() => useEmbeddedLinkInterceptor());
165+
166+
expect(restrictedLink).not.toHaveAttribute(GUARDED_LINK_ATTR);
167+
168+
act(() => {
169+
portalRoot.appendChild(createMlflowOwnedMarker());
170+
});
171+
172+
await waitFor(() => {
173+
expect(restrictedLink).toHaveAttribute(GUARDED_LINK_ATTR, 'true');
174+
});
175+
});
176+
});

0 commit comments

Comments
 (0)