Skip to content

Commit 52cf7ed

Browse files
Merge branch 'main' into fix/registry-name-field-validation
2 parents 36f330d + 4d8a1a0 commit 52cf7ed

66 files changed

Lines changed: 2457 additions & 112 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
---
2+
description: Theming external libraries (Perses, etc.) with PatternFly tokens in ODH Dashboard
3+
globs: "packages/observability/**,packages/*/src/**/*theme*,packages/*/src/**/*Theme*"
4+
alwaysApply: false
5+
---
6+
7+
# Third-Party Library Theming — ODH Dashboard
8+
9+
Reference implementation: `packages/observability/src/perses/theme.ts`
10+
11+
For the MLflow federated UI theming approach (Emotion token translation + SCSS shell overrides), see the [MLflow fork](https://github.com/opendatahub-io/mlflow/tree/master/mlflow/server/js/src/common/styles/patternfly) — that pattern lives in the fork, not here.
12+
13+
---
14+
15+
## Invariant 1 — ECharts/canvas: use `.value`, not CSS vars
16+
17+
Canvas-based renderers cannot resolve CSS custom properties at paint time. Passing a `var(--pf-t--...)` string to an ECharts option or canvas `fillStyle` silently produces the wrong color.
18+
19+
```ts
20+
import { t_color_white, t_color_gray_95 } from '@patternfly/react-tokens';
21+
22+
// Correct — resolved hex value
23+
const textColor = isDark ? t_color_white.value : t_color_gray_95.value;
24+
25+
// These overrides are passed to generateChartsTheme(), not as top-level MUI theme properties
26+
const chartsTheme = generateChartsTheme(muiTheme, {
27+
echartsTheme: {
28+
color: palette, // hex array, never CSS var strings
29+
tooltip: {
30+
// CSS vars ARE fine for non-canvas properties (e.g. tooltip is DOM-rendered)
31+
backgroundColor: 'var(--pf-t--global--background--color--inverse--default)',
32+
},
33+
},
34+
thresholds: {
35+
defaultColor: textColor, // .value, not .var
36+
},
37+
});
38+
```
39+
40+
Use `@patternfly/react-tokens` `.value` anywhere the value is consumed by a canvas renderer. Use `var(--pf-t--*)` strings everywhere else.
41+
42+
---
43+
44+
## Invariant 2 — Dark mode source: `useThemeContext()` only
45+
46+
Never infer dark mode from `prefers-color-scheme`, a local prop, or any source other than the dashboard theme context.
47+
48+
```ts
49+
import { useThemeContext } from '@odh-dashboard/internal/app/ThemeContext';
50+
51+
const { theme: contextTheme } = useThemeContext();
52+
const theme: PatternFlyTheme = contextTheme === 'dark' ? 'dark' : 'light';
53+
```
54+
55+
The full theme object must recompute when context changes:
56+
57+
```ts
58+
return React.useMemo(() => {
59+
const muiTheme = getTheme(theme, { ...mapPatternFlyThemeToMUI(theme) });
60+
// ... build chartsTheme ...
61+
return { muiTheme, chartsTheme };
62+
}, [theme]);
63+
```
64+
65+
---
66+
67+
## Invariant 3 — `ThemeProvider` at the library boundary, not the app root
68+
69+
Placing an MUI `ThemeProvider` at the app root leaks MUI styles into PF-only components. Scope it as close to the library's render boundary as possible.
70+
71+
```tsx
72+
// Correct — scoped to the Perses render boundary
73+
// packages/observability/src/perses/PersesWrapper.tsx
74+
const { muiTheme, chartsTheme } = usePatternFlyTheme();
75+
return (
76+
<ThemeProvider theme={muiTheme}>
77+
<ChartsProvider chartsTheme={chartsTheme}>
78+
{children}
79+
</ChartsProvider>
80+
</ThemeProvider>
81+
);
82+
83+
// Wrong — app root wrapping bleeds MUI styles into unrelated PF components
84+
// frontend/src/app/App.tsx
85+
return <ThemeProvider theme={muiTheme}><App /></ThemeProvider>;
86+
```
87+
88+
One wrapper component per external library. Never scatter `ThemeProvider` across individual pages.

.github/workflows/cypress-e2e-test.yml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -858,9 +858,9 @@ jobs:
858858
find ${{ github.workspace }}/packages/cypress/screenshots -type d -empty -delete 2>/dev/null || true
859859
find ${{ github.workspace }}/packages/cypress/videos -type d -empty -delete 2>/dev/null || true
860860
861-
# Fix read-only envtest binaries from previous BFF runs (setup-envtest downloads
862-
# k8s binaries with read-only permissions, which blocks actions/checkout cleanup)
863-
find ${{ github.workspace }}/packages/*/bff/bin -type f ! -perm -u+w -exec chmod u+w {} + 2>/dev/null || true
861+
# Fix read-only envtest binaries and directories from previous BFF runs
862+
# (setup-envtest downloads k8s binaries with 0555 permissions, which blocks actions/checkout cleanup)
863+
chmod -R u+w ${{ github.workspace }}/packages/*/bff/bin ${{ github.workspace }}/packages/*/upstream/bff/bin 2>/dev/null || true
864864
865865
echo "✅ Cleanup complete (non-critical, continued on any errors)"
866866
@@ -1418,8 +1418,8 @@ jobs:
14181418
echo "✅ Cleaned up $BFF_KILLED_COUNT BFF process(es) for run_id: ${{ github.run_id }}"
14191419
fi
14201420
1421-
# Fix read-only envtest binaries so next checkout can clean the workspace
1422-
find ${{ github.workspace }}/packages/*/bff/bin -type f ! -perm -u+w -exec chmod u+w {} + 2>/dev/null || true
1421+
# Fix read-only envtest binaries and directories so next checkout can clean the workspace
1422+
chmod -R u+w ${{ github.workspace }}/packages/*/bff/bin ${{ github.workspace }}/packages/*/upstream/bff/bin 2>/dev/null || true
14231423
14241424
- name: Stop Cypress Servers
14251425
run: |

AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ Rules live in `.claude/rules/`. Read the relevant rule file before starting the
9595
| **React** | `react.md` | When writing React components, hooks, or pages |
9696
| **Security** | `security.md` | When working on auth, secrets, input validation, or K8s API interactions |
9797
| **Testing Standards** | `testing-standards.md` | When working across multiple test types or choosing a testing strategy |
98+
| **Third-Party Theming** | `third-party-theming.md` | When theming external libraries (Perses, MLflow, etc.) or mapping PF tokens into non-PF component systems |
9899
| **Unit Tests** | `unit-tests.md` | When creating or modifying Jest unit tests for utilities, hooks, or components |
99100

100101
## Agent Skills

backend/src/utils/notebookUtils.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,18 @@ const getImageTag = (image: ImageInfo, imageTagName: string): ImageTag => {
137137
};
138138
};
139139

140+
const getMlflowAnnotation = (fastify: KubeFastifyInstance): Record<string, string> => {
141+
const isMlflowFlagEnabled = getDashboardConfig().spec.dashboardConfig.mlflow;
142+
if (!isMlflowFlagEnabled) {
143+
return {};
144+
}
145+
const dscStatus = getClusterStatus(fastify);
146+
if (dscStatus?.components?.mlflowoperator?.managementState === 'Managed') {
147+
return { 'opendatahub.io/mlflow-instance': 'mlflow' };
148+
}
149+
return {};
150+
};
151+
140152
export const assembleNotebook = async (
141153
fastify: KubeFastifyInstance,
142154
data: NotebookData,
@@ -226,6 +238,7 @@ export const assembleNotebook = async (
226238
'opendatahub.io/hardware-profile-name': selectedHardwareProfile?.metadata.name || '',
227239
'opendatahub.io/hardware-profile-namespace':
228240
selectedHardwareProfile?.metadata.namespace || '',
241+
...getMlflowAnnotation(fastify),
229242
},
230243
name: name,
231244
namespace: namespace,
@@ -399,6 +412,7 @@ export const createNotebook = async (
399412
}
400413

401414
notebookAssembled.metadata.annotations['notebooks.opendatahub.io/inject-auth'] = 'true';
415+
402416
const notebookContainers = notebookAssembled.spec.template.spec.containers;
403417

404418
if (!notebookContainers[0]) {

frontend/src/__mocks__/mockLLMInferenceServiceConfigK8sResource.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ type MockLLMInferenceServiceConfigType = {
1010
modelUri?: string;
1111
modelName?: string;
1212
templateName?: string;
13+
disabled?: boolean;
1314
};
1415

1516
export const mockLLMInferenceServiceConfigK8sResource = ({
@@ -22,6 +23,7 @@ export const mockLLMInferenceServiceConfigK8sResource = ({
2223
modelUri = 'hf://test/model',
2324
modelName = 'test-model',
2425
templateName,
26+
disabled,
2527
}: MockLLMInferenceServiceConfigType): LLMInferenceServiceConfigKind => ({
2628
apiVersion: 'serving.kserve.io/v1alpha1',
2729
kind: 'LLMInferenceServiceConfig',
@@ -35,6 +37,7 @@ export const mockLLMInferenceServiceConfigK8sResource = ({
3537
? { 'opendatahub.io/recommended-accelerators': recommendedAccelerators }
3638
: {}),
3739
...(templateName ? { 'opendatahub.io/template-name': templateName } : {}),
40+
...(disabled ? { 'opendatahub.io/disabled': 'true' as const } : {}),
3841
},
3942
labels: {
4043
'opendatahub.io/config-type': configType,

frontend/src/api/k8s/__tests__/notebooks.spec.ts

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import {
3434
updateNotebook,
3535
restartNotebook,
3636
patchNotebookImage,
37+
getMlflowInstancePatch,
3738
} from '#~/api/k8s/notebooks';
3839

3940
import {
@@ -406,6 +407,23 @@ describe('assembleNotebook', () => {
406407
expect(k8sCreateResourceMock).toHaveBeenCalledTimes(1);
407408
expect(renderResult).toStrictEqual(mockNotebookK8sResource({ uid: 'test' }));
408409
});
410+
411+
it('should include mlflow-instance annotation when mlflowEnabled is true', () => {
412+
const notebookData = mockStartNotebookData({});
413+
notebookData.mlflowEnabled = true;
414+
const result = assembleNotebook(notebookData, 'test-user');
415+
expect(result.metadata.annotations?.['opendatahub.io/mlflow-instance']).toBe('mlflow');
416+
});
417+
418+
it.each([false, undefined])(
419+
'should not include mlflow-instance annotation when mlflowEnabled is %s',
420+
(mlflowEnabled) => {
421+
const notebookData = mockStartNotebookData({});
422+
notebookData.mlflowEnabled = mlflowEnabled;
423+
const result = assembleNotebook(notebookData, 'test-user');
424+
expect(result.metadata.annotations?.['opendatahub.io/mlflow-instance']).toBeUndefined();
425+
},
426+
);
409427
});
410428

411429
describe('createNotebook', () => {
@@ -1429,3 +1447,55 @@ describe('updateNotebook', () => {
14291447
expect(shmMount?.mountPath).toBe('/dev/shm');
14301448
});
14311449
});
1450+
1451+
describe('getMlflowInstancePatch', () => {
1452+
it('should return an add patch when mlflow is enabled and annotation is absent', () => {
1453+
const notebook = mockNotebookK8sResource({});
1454+
const patches = getMlflowInstancePatch(notebook, true);
1455+
expect(patches).toEqual([
1456+
{
1457+
op: 'add',
1458+
path: '/metadata/annotations/opendatahub.io~1mlflow-instance',
1459+
value: 'mlflow',
1460+
},
1461+
]);
1462+
});
1463+
1464+
it('should initialize annotations when absent before adding mlflow annotation', () => {
1465+
const notebook = mockNotebookK8sResource({});
1466+
notebook.metadata.annotations = undefined;
1467+
const patches = getMlflowInstancePatch(notebook, true);
1468+
expect(patches).toEqual([
1469+
{
1470+
op: 'add',
1471+
path: '/metadata/annotations',
1472+
value: {},
1473+
},
1474+
{
1475+
op: 'add',
1476+
path: '/metadata/annotations/opendatahub.io~1mlflow-instance',
1477+
value: 'mlflow',
1478+
},
1479+
]);
1480+
});
1481+
1482+
it('should return empty patches when the annotation already exists', () => {
1483+
const notebook = mockNotebookK8sResource({
1484+
opts: {
1485+
metadata: {
1486+
annotations: {
1487+
'opendatahub.io/mlflow-instance': 'mlflow',
1488+
},
1489+
},
1490+
},
1491+
});
1492+
const patches = getMlflowInstancePatch(notebook, true);
1493+
expect(patches).toEqual([]);
1494+
});
1495+
1496+
it('should return empty patches when mlflow is not enabled', () => {
1497+
const notebook = mockNotebookK8sResource({});
1498+
const patches = getMlflowInstancePatch(notebook, false);
1499+
expect(patches).toEqual([]);
1500+
});
1501+
});

frontend/src/api/k8s/notebooks.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ export const assembleNotebook = (
4848
connections,
4949
hardwareProfileOptions,
5050
feastData,
51+
mlflowEnabled,
5152
} = data;
5253
const {
5354
name: notebookName,
@@ -110,6 +111,7 @@ export const assembleNotebook = (
110111
image.imageVersion?.annotations?.['opendatahub.io/notebook-build-commit'] ?? '',
111112
'opendatahub.io/connections': connectionsAnnotation ?? '',
112113
...(feastData?.annotations || {}),
114+
...(mlflowEnabled ? { 'opendatahub.io/mlflow-instance': 'mlflow' } : {}),
113115
},
114116
name: notebookId,
115117
namespace: projectName,
@@ -204,6 +206,26 @@ export const getStopPatch = (): Patch => ({
204206
value: getStopPatchDataString(),
205207
});
206208

209+
export const getMlflowInstancePatch = (notebook: NotebookKind, mlflowEnabled: boolean): Patch[] => {
210+
if (!mlflowEnabled || notebook.metadata.annotations?.['opendatahub.io/mlflow-instance']) {
211+
return [];
212+
}
213+
const patches: Patch[] = [];
214+
if (!notebook.metadata.annotations) {
215+
patches.push({
216+
op: 'add',
217+
path: '/metadata/annotations',
218+
value: {},
219+
});
220+
}
221+
patches.push({
222+
op: 'add',
223+
path: '/metadata/annotations/opendatahub.io~1mlflow-instance',
224+
value: 'mlflow',
225+
});
226+
return patches;
227+
};
228+
207229
export const getNotebooks = (namespace: string): Promise<NotebookKind[]> =>
208230
k8sListResource<NotebookKind>({
209231
model: NotebookModel,
Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,27 @@
1-
import { HelperText, HelperTextItem } from '@patternfly/react-core';
1+
import { FormHelperText, HelperText, HelperTextItem } from '@patternfly/react-core';
22
import React from 'react';
33

44
interface CharLimitHelperTextProps {
55
limit: number;
6+
currentLength: number;
67
}
78

8-
export const CharLimitHelperText: React.FC<CharLimitHelperTextProps> = ({ limit }) => (
9-
<HelperText>
10-
<HelperTextItem>{`Cannot exceed ${limit} characters`}</HelperTextItem>
11-
</HelperText>
12-
);
9+
export const CharLimitHelperText: React.FC<CharLimitHelperTextProps> = ({
10+
limit,
11+
currentLength,
12+
}) => {
13+
// Threshold of 10 is consistent with K8sNameDescriptionField (showNameWarning),
14+
// which uses `maxLength - 10` to only warn when the user is close to the limit.
15+
if (currentLength < limit - 10) {
16+
return null;
17+
}
18+
return (
19+
<FormHelperText>
20+
<HelperText>
21+
<HelperTextItem variant={currentLength >= limit ? 'error' : 'warning'}>
22+
{`Cannot exceed ${limit} characters (${Math.max(0, limit - currentLength)} remaining)`}
23+
</HelperTextItem>
24+
</HelperText>
25+
</FormHelperText>
26+
);
27+
};

frontend/src/concepts/areas/const.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,7 @@ export const SupportedAreasStateMap: SupportedAreasState = {
215215
},
216216
[SupportedArea.MLFLOW]: {
217217
featureFlags: ['mlflow'],
218+
requiredComponents: [DataScienceStackComponent.MLFLOW],
218219
},
219220
[SupportedArea.PROJECT_RBAC_SETTINGS]: {
220221
featureFlags: ['projectRBAC'],
@@ -241,4 +242,5 @@ export const DataScienceStackComponentMap: Record<string, string> = {
241242
[DataScienceStackComponent.TRUSTY_AI]: 'TrustyAI',
242243
[DataScienceStackComponent.WORKBENCHES]: 'Workbenches',
243244
[DataScienceStackComponent.TRAINER]: 'Trainer',
245+
[DataScienceStackComponent.MLFLOW]: 'MLflow',
244246
};

frontend/src/concepts/areas/types.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@ export enum DataScienceStackComponent {
117117
WORKBENCHES = 'workbenches',
118118
LLAMA_STACK_OPERATOR = 'llamastackoperator',
119119
TRAINER = 'trainer',
120+
MLFLOW = 'mlflowoperator',
120121
}
121122

122123
/**

0 commit comments

Comments
 (0)