Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
EditColumnsMenu,
} from '@nemo/common/src/components/DataView/internal';
import {
ROW_ACTIONS_COLUMN_SIZE,
ROW_SELECTION_COLUMN_SIZE,
StudioDataView,
} from '@nemo/common/src/components/DataView/StudioDataView';
Expand Down Expand Up @@ -35,11 +36,12 @@ import {
} from '@studio/components/dataViews/ExperimentGroupDataView/useExperimentGroupEvaluations';
import { useSortErrorRecovery } from '@studio/components/dataViews/ExperimentGroupDataView/useSortErrorRecovery';
import { deriveEvaluatorNames } from '@studio/components/dataViews/ExperimentGroupDataView/util';
import { QuickActionsMenuRoot } from '@studio/components/QuickActionsMenu/QuickActionsMenuRoot';
import { useWorkspaceFromPath } from '@studio/hooks/useWorkspaceFromPath';
import { getEvaluationDetailRoute } from '@studio/routes/utils';
import { tooltipClassName } from '@studio/styles/common';
import { useLocalStorage } from '@studio/util/hooks/useLocalStorage';
import { Columns3, FolderPlus, Pin } from 'lucide-react';
import { Columns3, FolderMinus, FolderPlus, Pin } from 'lucide-react';
import { type ComponentProps, type FC, useCallback, useEffect, useMemo, useState } from 'react';
import { useNavigate } from 'react-router-dom';

Expand Down Expand Up @@ -169,8 +171,9 @@ export const ExperimentGroupDataView: FC<ExperimentGroupDataViewProps> = ({
// The evaluations leaderboard supports multi-column sort (shift-click) — score vs. cost etc.
multiSort: true,
columnVisibility: { created_by: false, updated_at: false },
// Keep the pin toggle and row selection reachable while horizontally scrolling this wide table.
columnPinning: { left: ['pin', 'row-selection'] },
// Keep the pin toggle, row selection, and the row actions menu reachable while horizontally
// scrolling this wide table.
columnPinning: { left: ['pin', 'row-selection'], right: ['row-actions'] },
filterFieldMap: getEvaluationFilterField,
columnOrder: savedColumnOrder ?? [],
});
Expand All @@ -188,6 +191,7 @@ export const ExperimentGroupDataView: FC<ExperimentGroupDataViewProps> = ({
const {
rows: orderedData,
togglePin,
removeFromGroup,
totalCount,
error,
isLoading,
Expand Down Expand Up @@ -235,7 +239,7 @@ export const ExperimentGroupDataView: FC<ExperimentGroupDataViewProps> = ({
const makeColumns = useCallback<
ComponentProps<typeof DataViewRoot<EvaluationRow>>['makeColumns']
>(
({ accessor, display }, { rowSelectionColumn }) => [
({ accessor, display }, { rowSelectionColumn, rowActionsColumn }) => [
rowSelectionColumn({ size: ROW_SELECTION_COLUMN_SIZE }),
display({
id: 'pin',
Expand Down Expand Up @@ -461,8 +465,31 @@ export const ExperimentGroupDataView: FC<ExperimentGroupDataViewProps> = ({
filter: { type: 'text', label: 'Created By', placeholder: 'Filter by Created By' },
},
}),
rowActionsColumn({
size: ROW_ACTIONS_COLUMN_SIZE,
enableResizing: false,
cell: ({ row }) => (
<QuickActionsMenuRoot
actions={[
{
// `fill-none` overrides the menu's cloned `fill: 'solid'`, which would otherwise paint
// the folder body over the "+"/"-" (they're drawn before the folder shape).
label: 'Add to group',
icon: <FolderPlus className="fill-none" size={16} />,
onSelect: () =>
setAddToGroupState({ evaluations: [row.original], clearSelection: () => {} }),
},
{
label: 'Remove from group',
icon: <FolderMinus className="fill-none" size={16} />,
onSelect: () => removeFromGroup(row.original),
},
]}
/>
),
}),
],
[evaluatorNames, togglePin, metadataKeys]
[evaluatorNames, togglePin, removeFromGroup, metadataKeys]
);

// A recoverable sort error is handled by useSortErrorRecovery (toast + revert), and the table keeps
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ import { useToast } from '@nemo/common/src/providers/toast/useToast';
import {
getListEvaluationsQueryKey,
useListEvaluations,
usePatchEvaluation,
usePinEvaluation,
useUnpinEvaluation,
} from '@nemo/sdk/generated/platform/api';
import {
type EvaluationRow,
useExperimentGroupEvaluations,
type UseExperimentGroupEvaluationsParams,
} from '@studio/components/dataViews/ExperimentGroupDataView/useExperimentGroupEvaluations';
Expand All @@ -27,6 +29,7 @@ const mockUseToast = vi.mocked(useToast);
const mockUseListEvaluations = vi.mocked(useListEvaluations);
const mockUsePinEvaluation = vi.mocked(usePinEvaluation);
const mockUseUnpinEvaluation = vi.mocked(useUnpinEvaluation);
const mockUsePatchEvaluation = vi.mocked(usePatchEvaluation);
const mockGetListEvaluationsQueryKey = vi.mocked(getListEvaluationsQueryKey);

interface Row {
Expand Down Expand Up @@ -79,6 +82,9 @@ describe('useExperimentGroupEvaluations', () => {
mockUseUnpinEvaluation.mockReturnValue({
mutate: vi.fn(),
} as unknown as ReturnType<typeof useUnpinEvaluation>);
mockUsePatchEvaluation.mockReturnValue({
mutate: vi.fn(),
} as unknown as ReturnType<typeof usePatchEvaluation>);
});

it('paginates over the unpinned set only, so pinned rows do not inflate the page count', () => {
Expand Down Expand Up @@ -224,4 +230,43 @@ describe('useExperimentGroupEvaluations', () => {
});
expect(invalidateQueries).toHaveBeenCalledTimes(1);
});

it('blocks removing an evaluation whose only group is this one, without patching', () => {
mockLists({ rows: [], total: 0 }, { rows: [], total: 0 });
const patch = vi.fn();
mockUsePatchEvaluation.mockReturnValue({
mutate: patch,
} as unknown as ReturnType<typeof usePatchEvaluation>);
const error = vi.fn();
mockUseToast.mockReturnValue({ error } as unknown as ReturnType<typeof useToast>);

const { result } = renderHook(() => useExperimentGroupEvaluations(baseParams));
result.current.removeFromGroup({
name: 'only-here',
experiment_ids: ['grp'],
} as unknown as EvaluationRow);

expect(patch).not.toHaveBeenCalled();
expect(error).toHaveBeenCalled();
});

it('removes this group from an evaluation that has others, patching the remaining ids', () => {
mockLists({ rows: [], total: 0 }, { rows: [], total: 0 });
const patch = vi.fn();
mockUsePatchEvaluation.mockReturnValue({
mutate: patch,
} as unknown as ReturnType<typeof usePatchEvaluation>);

const { result } = renderHook(() => useExperimentGroupEvaluations(baseParams));
result.current.removeFromGroup({
name: 'multi',
experiment_ids: ['grp', 'other'],
} as unknown as EvaluationRow);

expect(patch).toHaveBeenCalledWith({
workspace: 'ws',
name: 'multi',
data: { experiment_ids: ['other'] },
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
getListEvaluationsQueryKey,
type ListEvaluationsQueryError,
useListEvaluations,
usePatchEvaluation,
usePinEvaluation,
useUnpinEvaluation,
} from '@nemo/sdk/generated/platform/api';
Expand Down Expand Up @@ -50,6 +51,9 @@ export interface ExperimentGroupEvaluations {
rows: EvaluationRow[];
/** Pins the row if unpinned, unpins it otherwise, then refetches both lists. */
togglePin: (row: EvaluationRow) => void;
/** Removes the current group from the evaluation's `experiment_ids`, then refetches. Blocks (with a
* toast) when this is the evaluation's only group — every evaluation must belong to at least one. */
removeFromGroup: (row: EvaluationRow) => void;
/**
* Row count that drives pagination: the unpinned total. Pinned rows ride atop every page and are
* not paginated, so they're excluded; falls back to the pinned count when nothing is unpinned so
Expand Down Expand Up @@ -186,6 +190,39 @@ export function useExperimentGroupEvaluations({
[workspace, pinEvaluation, unpinEvaluation]
);

const { mutate: patchEvaluation } = usePatchEvaluation({
mutation: {
onSuccess: () => {
toast.success('Removed the evaluation from this group.');
return invalidateList();
},
onError: () => toast.error('Failed to remove the evaluation from this group.'),
onSettled: (_data, _error, { name }) => {
pendingRef.current.delete(name);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
},
},
});

const removeFromGroup = useCallback(
(row: EvaluationRow) => {
const { name } = row;
if (pendingRef.current.has(name)) return;
// The API rejects an empty `experiment_ids`, so block removing an evaluation's only group and
// explain why rather than firing a request that would 400.
if (row.experiment_ids.length <= 1) {
toast.error('This evaluation must belong to at least one experiment group.');
return;
}
pendingRef.current.add(name);
patchEvaluation({
workspace,
name,
data: { experiment_ids: row.experiment_ids.filter((id) => id !== experimentGroupId) },
});
},
[workspace, experimentGroupId, patchEvaluation, toast]
);

// Pinned first, then the current page of unpinned. Drop any unpinned row already shown as pinned —
// it can appear in both server lists during the brief window where the two queries refetch out of step.
const rows = useMemo<EvaluationRow[]>(() => {
Expand All @@ -209,5 +246,5 @@ export function useExperimentGroupEvaluations({
// `keepPreviousData`'s in-flight 'success' doesn't bank an about-to-fail sort as the last good one.
const isSuccess = isUnpinnedSuccess && !isUnpinnedPlaceholder;

return { rows, togglePin, totalCount, error, isLoading, isFetching, isSuccess };
return { rows, togglePin, removeFromGroup, totalCount, error, isLoading, isFetching, isSuccess };
}