Skip to content

Commit ca048d8

Browse files
jeffspahrjeffspahr
authored
refactor(frontend): simplify CompareV2 selection state (#13256)
* refactor(frontend): simplify CompareV2 selection state Signed-off-by: Jeff Spahr <spahrj@gmail.com> * fix(frontend): refresh CompareV2 artifact selection Signed-off-by: Jeff Spahr <spahrj@gmail.com> * test(frontend): cover CompareV2 route artifact refresh Signed-off-by: Jeff Spahr <spahrj@gmail.com> * fix(frontend): share keyed compare selection state Signed-off-by: Jeff Spahr <spahrj@gmail.com> --------- Signed-off-by: Jeff Spahr <spahrj@gmail.com>
1 parent 24bd153 commit ca048d8

6 files changed

Lines changed: 361 additions & 75 deletions

File tree

‎frontend/src/components/viewers/MetricsDropdown.test.tsx‎

Lines changed: 158 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import * as Utils from 'src/lib/Utils';
2323
import { SelectedArtifact } from 'src/pages/CompareV2';
2424
import { LinkedArtifact } from 'src/mlmd/MlmdUtils';
2525
import * as jspb from 'google-protobuf';
26+
import { useState } from 'react';
2627
import MetricsDropdown from './MetricsDropdown';
2728
import { MetricsType, RunArtifact } from 'src/lib/v2/CompareUtils';
2829

@@ -76,6 +77,38 @@ function newMockLinkedArtifact(id: number, displayName?: string): LinkedArtifact
7677
} as LinkedArtifact;
7778
}
7879

80+
interface ControlledMetricsDropdownProps {
81+
filteredRunArtifacts: RunArtifact[];
82+
metricsTab: MetricsType;
83+
selectedArtifacts: SelectedArtifact[];
84+
namespace?: string;
85+
onUpdateSelectedArtifacts?: (selectedArtifacts: SelectedArtifact[]) => void;
86+
}
87+
88+
function ControlledMetricsDropdown(props: ControlledMetricsDropdownProps) {
89+
const {
90+
filteredRunArtifacts,
91+
metricsTab,
92+
selectedArtifacts: initialSelectedArtifacts,
93+
namespace,
94+
onUpdateSelectedArtifacts,
95+
} = props;
96+
const [selectedArtifacts, setSelectedArtifacts] = useState(initialSelectedArtifacts);
97+
98+
return (
99+
<MetricsDropdown
100+
filteredRunArtifacts={filteredRunArtifacts}
101+
metricsTab={metricsTab}
102+
selectedArtifacts={selectedArtifacts}
103+
updateSelectedArtifacts={(nextSelectedArtifacts) => {
104+
setSelectedArtifacts(nextSelectedArtifacts);
105+
onUpdateSelectedArtifacts?.(nextSelectedArtifacts);
106+
}}
107+
namespace={namespace}
108+
/>
109+
);
110+
}
111+
79112
testBestPractices();
80113
describe('MetricsDropdown', () => {
81114
const updateSelectedArtifactsSpy = vi.fn();
@@ -85,6 +118,7 @@ describe('MetricsDropdown', () => {
85118
let scalarMetricsArtifacts: RunArtifact[];
86119

87120
beforeEach(() => {
121+
updateSelectedArtifactsSpy.mockReset();
88122
emptySelectedArtifacts = [
89123
{
90124
selectedItem: { itemName: '', subItemName: '' },
@@ -157,11 +191,11 @@ describe('MetricsDropdown', () => {
157191
it('Dropdown loaded when content is present', async () => {
158192
render(
159193
<CommonTestWrapper>
160-
<MetricsDropdown
194+
<ControlledMetricsDropdown
161195
filteredRunArtifacts={scalarMetricsArtifacts}
162196
metricsTab={MetricsType.CONFUSION_MATRIX}
163197
selectedArtifacts={emptySelectedArtifacts}
164-
updateSelectedArtifacts={updateSelectedArtifactsSpy}
198+
onUpdateSelectedArtifacts={updateSelectedArtifactsSpy}
165199
/>
166200
</CommonTestWrapper>,
167201
);
@@ -251,11 +285,11 @@ describe('MetricsDropdown', () => {
251285

252286
render(
253287
<CommonTestWrapper>
254-
<MetricsDropdown
288+
<ControlledMetricsDropdown
255289
filteredRunArtifacts={scalarMetricsArtifacts}
256290
metricsTab={MetricsType.HTML}
257291
selectedArtifacts={emptySelectedArtifacts}
258-
updateSelectedArtifacts={updateSelectedArtifactsSpy}
292+
onUpdateSelectedArtifacts={updateSelectedArtifactsSpy}
259293
/>
260294
</CommonTestWrapper>,
261295
);
@@ -296,11 +330,11 @@ describe('MetricsDropdown', () => {
296330

297331
render(
298332
<CommonTestWrapper>
299-
<MetricsDropdown
333+
<ControlledMetricsDropdown
300334
filteredRunArtifacts={scalarMetricsArtifacts}
301335
metricsTab={MetricsType.MARKDOWN}
302336
selectedArtifacts={emptySelectedArtifacts}
303-
updateSelectedArtifacts={updateSelectedArtifactsSpy}
337+
onUpdateSelectedArtifacts={updateSelectedArtifactsSpy}
304338
/>
305339
</CommonTestWrapper>,
306340
);
@@ -345,11 +379,11 @@ describe('MetricsDropdown', () => {
345379

346380
render(
347381
<CommonTestWrapper>
348-
<MetricsDropdown
382+
<ControlledMetricsDropdown
349383
filteredRunArtifacts={scalarMetricsArtifacts}
350384
metricsTab={MetricsType.HTML}
351385
selectedArtifacts={emptySelectedArtifacts}
352-
updateSelectedArtifacts={updateSelectedArtifactsSpy}
386+
onUpdateSelectedArtifacts={updateSelectedArtifactsSpy}
353387
namespace='namespaceInput'
354388
/>
355389
</CommonTestWrapper>,
@@ -391,11 +425,11 @@ describe('MetricsDropdown', () => {
391425

392426
render(
393427
<CommonTestWrapper>
394-
<MetricsDropdown
428+
<ControlledMetricsDropdown
395429
filteredRunArtifacts={scalarMetricsArtifacts}
396430
metricsTab={MetricsType.CONFUSION_MATRIX}
397431
selectedArtifacts={newSelectedArtifacts}
398-
updateSelectedArtifacts={updateSelectedArtifactsSpy}
432+
onUpdateSelectedArtifacts={updateSelectedArtifactsSpy}
399433
/>
400434
</CommonTestWrapper>,
401435
);
@@ -404,4 +438,118 @@ describe('MetricsDropdown', () => {
404438
screen.getByText('Choose a first Confusion Matrix artifact');
405439
screen.getByLabelText('run1 > execution1 > artifact1');
406440
});
441+
442+
it('updates the displayed selection when parent-selected artifacts change after mount', async () => {
443+
const { rerender } = render(
444+
<CommonTestWrapper>
445+
<MetricsDropdown
446+
filteredRunArtifacts={scalarMetricsArtifacts}
447+
metricsTab={MetricsType.CONFUSION_MATRIX}
448+
selectedArtifacts={emptySelectedArtifacts}
449+
updateSelectedArtifacts={updateSelectedArtifactsSpy}
450+
/>
451+
</CommonTestWrapper>,
452+
);
453+
await TestUtils.flushPromises();
454+
455+
const nextSelectedArtifacts: SelectedArtifact[] = [
456+
{
457+
linkedArtifact: firstLinkedArtifact,
458+
selectedItem: {
459+
itemName: 'run1',
460+
subItemName: 'execution1',
461+
subItemSecondaryName: 'artifact1',
462+
},
463+
},
464+
emptySelectedArtifacts[1],
465+
];
466+
467+
rerender(
468+
<CommonTestWrapper>
469+
<MetricsDropdown
470+
filteredRunArtifacts={scalarMetricsArtifacts}
471+
metricsTab={MetricsType.CONFUSION_MATRIX}
472+
selectedArtifacts={nextSelectedArtifacts}
473+
updateSelectedArtifacts={updateSelectedArtifactsSpy}
474+
/>
475+
</CommonTestWrapper>,
476+
);
477+
478+
expect(await screen.findByLabelText('run1 > execution1 > artifact1')).toBeInTheDocument();
479+
});
480+
481+
it('re-resolves the selected artifact from the current compare data when labels stay the same', async () => {
482+
const getHtmlViewerConfigSpy = vi.spyOn(metricsVisualizations, 'getHtmlViewerConfig');
483+
getHtmlViewerConfigSpy.mockResolvedValue([]);
484+
485+
const staleLinkedArtifact = newMockLinkedArtifact(10, 'artifact1');
486+
const freshLinkedArtifact = newMockLinkedArtifact(11, 'artifact1');
487+
const selectedArtifactsWithStaleArtifact: SelectedArtifact[] = [
488+
{
489+
linkedArtifact: staleLinkedArtifact,
490+
selectedItem: {
491+
itemName: 'run1',
492+
subItemName: 'execution1',
493+
subItemSecondaryName: 'artifact1',
494+
},
495+
},
496+
emptySelectedArtifacts[1],
497+
];
498+
499+
const { rerender } = render(
500+
<CommonTestWrapper>
501+
<MetricsDropdown
502+
filteredRunArtifacts={[
503+
{
504+
run: {
505+
run_id: '1',
506+
display_name: 'run1',
507+
},
508+
executionArtifacts: [
509+
{
510+
execution: newMockExecution(1, 'execution1'),
511+
linkedArtifacts: [staleLinkedArtifact],
512+
},
513+
],
514+
},
515+
]}
516+
metricsTab={MetricsType.HTML}
517+
selectedArtifacts={selectedArtifactsWithStaleArtifact}
518+
updateSelectedArtifacts={updateSelectedArtifactsSpy}
519+
/>
520+
</CommonTestWrapper>,
521+
);
522+
await TestUtils.flushPromises();
523+
await waitFor(() => {
524+
expect(getHtmlViewerConfigSpy).toHaveBeenLastCalledWith([staleLinkedArtifact], undefined);
525+
});
526+
527+
rerender(
528+
<CommonTestWrapper>
529+
<MetricsDropdown
530+
filteredRunArtifacts={[
531+
{
532+
run: {
533+
run_id: '2',
534+
display_name: 'run1',
535+
},
536+
executionArtifacts: [
537+
{
538+
execution: newMockExecution(2, 'execution1'),
539+
linkedArtifacts: [freshLinkedArtifact],
540+
},
541+
],
542+
},
543+
]}
544+
metricsTab={MetricsType.HTML}
545+
selectedArtifacts={selectedArtifactsWithStaleArtifact}
546+
updateSelectedArtifacts={updateSelectedArtifactsSpy}
547+
/>
548+
</CommonTestWrapper>,
549+
);
550+
551+
await waitFor(() => {
552+
expect(getHtmlViewerConfigSpy).toHaveBeenLastCalledWith([freshLinkedArtifact], undefined);
553+
});
554+
});
407555
});

‎frontend/src/components/viewers/MetricsDropdown.tsx‎

Lines changed: 14 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -92,39 +92,21 @@ export default function MetricsDropdown(props: MetricsDropdownProps) {
9292
updateSelectedArtifacts,
9393
namespace,
9494
} = props;
95-
const [firstSelectedItem, setFirstSelectedItem] = useState<SelectedItem>(
96-
selectedArtifacts[0].selectedItem,
97-
);
98-
const [secondSelectedItem, setSecondSelectedItem] = useState<SelectedItem>(
99-
selectedArtifacts[1].selectedItem,
100-
);
101-
102-
useEffect(() => {
103-
setFirstSelectedItem(selectedArtifacts[0].selectedItem);
104-
setSecondSelectedItem(selectedArtifacts[1].selectedItem);
105-
}, [selectedArtifacts]);
10695

10796
const selectedArtifactsForDisplay = useMemo(
108-
() => [
109-
{
110-
selectedItem: firstSelectedItem,
111-
linkedArtifact: getLinkedArtifactFromSelectedItem(filteredRunArtifacts, firstSelectedItem),
112-
},
113-
{
114-
selectedItem: secondSelectedItem,
115-
linkedArtifact: getLinkedArtifactFromSelectedItem(filteredRunArtifacts, secondSelectedItem),
116-
},
117-
],
118-
[filteredRunArtifacts, firstSelectedItem, secondSelectedItem],
97+
() =>
98+
selectedArtifacts.map((selectedArtifact) => ({
99+
selectedItem: selectedArtifact.selectedItem,
100+
linkedArtifact: getLinkedArtifactFromSelectedItem(
101+
filteredRunArtifacts,
102+
selectedArtifact.selectedItem,
103+
),
104+
})),
105+
[filteredRunArtifacts, selectedArtifacts],
119106
);
120107

121108
const metricsTabText = metricsTypeToString(metricsTab);
122-
const updateSelectedItemAndArtifact = (
123-
setSelectedItem: (selectedItem: SelectedItem) => void,
124-
panelIndex: number,
125-
selectedItem: SelectedItem,
126-
): void => {
127-
setSelectedItem(selectedItem);
109+
const updateSelectedItemAndArtifact = (panelIndex: number, selectedItem: SelectedItem): void => {
128110
const linkedArtifact = getLinkedArtifactFromSelectedItem(filteredRunArtifacts, selectedItem);
129111
const nextSelectedArtifacts = selectedArtifactsForDisplay.map((selectedArtifact, index) =>
130112
index === panelIndex
@@ -150,8 +132,8 @@ export default function MetricsDropdown(props: MetricsDropdownProps) {
150132
<TwoLevelDropdown
151133
title={`Choose a first ${metricsTabText} artifact`}
152134
items={dropdownItems}
153-
selectedItem={firstSelectedItem}
154-
setSelectedItem={updateSelectedItemAndArtifact.bind(null, setFirstSelectedItem, 0)}
135+
selectedItem={selectedArtifactsForDisplay[0].selectedItem}
136+
setSelectedItem={updateSelectedItemAndArtifact.bind(null, 0)}
155137
/>
156138
<VisualizationPanelItem
157139
metricsTab={metricsTab}
@@ -164,8 +146,8 @@ export default function MetricsDropdown(props: MetricsDropdownProps) {
164146
<TwoLevelDropdown
165147
title={`Choose a second ${metricsTabText} artifact`}
166148
items={dropdownItems}
167-
selectedItem={secondSelectedItem}
168-
setSelectedItem={updateSelectedItemAndArtifact.bind(null, setSecondSelectedItem, 1)}
149+
selectedItem={selectedArtifactsForDisplay[1].selectedItem}
150+
setSelectedItem={updateSelectedItemAndArtifact.bind(null, 1)}
169151
/>
170152
<VisualizationPanelItem
171153
metricsTab={metricsTab}

‎frontend/src/hooks/useKeyedState.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/*
2+
* Copyright 2026 The Kubeflow Authors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
import { useCallback, useState } from 'react';
18+
19+
type KeyedState<T> = {
20+
key: string;
21+
value: T;
22+
};
23+
24+
export function useKeyedState<T>(key: string, initialValue: T) {
25+
const [state, setState] = useState<KeyedState<T>>({ key: '', value: initialValue });
26+
const value = state.key === key ? state.value : initialValue;
27+
const setValue = useCallback((nextValue: T) => setState({ key, value: nextValue }), [key]);
28+
29+
return [value, setValue] as const;
30+
}

0 commit comments

Comments
 (0)