diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index 2077b57775..7eede5b21e 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -746,8 +746,8 @@ describe('App', () => { const menuitems = screen.getAllByRole('menuitem') const itemLabels = menuitems.map(item => item.textContent) expect(itemLabels).toStrictEqual([ - 'Modify and Resume', 'Modify, Reset and Run', + 'Modify and Resume', 'Modify and Queue', 'Star Experiment' ]) @@ -842,6 +842,137 @@ describe('App', () => { const itemLabels = menuitems.map(item => item.textContent) expect(itemLabels).toContain('Star Experiments') }) + + it('should allow batch selection from the bottom up too', () => { + render() + + fireEvent( + window, + new MessageEvent('message', { + data: { + data: { + ...tableDataFixture, + hasRunningExperiment: false + }, + type: MessageToWebviewType.SET_DATA + } + }) + ) + + const firstRowCheckbox = within(getRow('4fb124a')).getByRole('checkbox') + const tailRow = within(getRow('42b8736')).getByRole('checkbox') + fireEvent.click(tailRow) + fireEvent.click(firstRowCheckbox, { shiftKey: true }) + + const selectedRows = () => screen.getAllByRole('row', { selected: true }) + expect(selectedRows()).toHaveLength(4) + + const anotherRow = within(getRow('2173124')).getByRole('checkbox') + fireEvent.click(anotherRow, { shiftKey: true }) + expect(selectedRows()).toHaveLength(5) + }) + + it('should not include collapsed experiments in the bulk selection', () => { + render() + + fireEvent( + window, + new MessageEvent('message', { + data: { + data: { + ...tableDataFixture, + hasRunningExperiment: false + }, + type: MessageToWebviewType.SET_DATA + } + }) + ) + + const testBranchContractButton = within(getRow('42b8736')).getByTitle( + 'Contract Row' + ) + fireEvent.click(testBranchContractButton) + + jest.advanceTimersByTime(100) + const firstRowCheckbox = within(getRow('4fb124a')).getByRole('checkbox') + fireEvent.click(firstRowCheckbox) + + const tailRow = within(getRow('22e40e1')).getByRole('checkbox') + fireEvent.click(tailRow, { shiftKey: true }) + + fireEvent.click(testBranchContractButton) + + jest.advanceTimersByTime(100) + expect(getRow('42b8736')).toHaveAttribute('aria-selected', 'true') + expect(getRow('2173124')).not.toHaveAttribute('aria-selected', 'true') + expect(getRow('9523bde')).not.toHaveAttribute('aria-selected', 'true') + }) + + it('should present the Clear selected rows option when multiple rows are selected', () => { + render() + + fireEvent( + window, + new MessageEvent('message', { + data: { + data: { + ...tableDataFixture, + hasRunningExperiment: false + }, + type: MessageToWebviewType.SET_DATA + } + }) + ) + + const firstRowCheckbox = within(getRow('4fb124a')).getByRole('checkbox') + fireEvent.click(firstRowCheckbox) + + const tailRow = within(getRow('42b8736')).getByRole('checkbox') + fireEvent.click(tailRow, { shiftKey: true }) + + const selectedRows = () => + screen.queryAllByRole('row', { selected: true }) + expect(selectedRows().length).toBe(4) + + const target = screen.getByText('4fb124a') + fireEvent.contextMenu(target, { bubbles: true }) + + jest.advanceTimersByTime(100) + const clearOption = screen.getByText('Clear row selection') + fireEvent.click(clearOption) + + jest.advanceTimersByTime(100) + expect(selectedRows().length).toBe(0) + }) + + it('should clear the row selection when the Escape key is pressed', () => { + render() + + fireEvent( + window, + new MessageEvent('message', { + data: { + data: tableDataFixture, + type: MessageToWebviewType.SET_DATA + } + }) + ) + + const firstRowCheckbox = within(getRow('4fb124a')).getByRole('checkbox') + fireEvent.click(firstRowCheckbox) + + const tailRow = within(getRow('42b8736')).getByRole('checkbox') + fireEvent.click(tailRow, { shiftKey: true }) + + const selectedRows = () => + screen.queryAllByRole('row', { selected: true }) + expect(selectedRows().length).toBe(4) + + fireEvent.keyUp(tailRow, { bubbles: true, key: 'Escape' }) + + jest.advanceTimersByTime(100) + expect(selectedRows().length).toBe(0) + }) }) describe('Star Experiments', () => { diff --git a/webview/src/experiments/components/table/Row.tsx b/webview/src/experiments/components/table/Row.tsx index c84b630dff..7991374c5a 100644 --- a/webview/src/experiments/components/table/Row.tsx +++ b/webview/src/experiments/components/table/Row.tsx @@ -46,7 +46,10 @@ const experimentMenuOption = ( } as MessagesMenuOptionProps } -const getMultiSelectMenuOptions = (selectedRowsList: RowProp[]) => { +const getMultiSelectMenuOptions = ( + selectedRowsList: RowProp[], + hasRunningExperiment: boolean +) => { const unstarredExperiments = selectedRowsList.filter( ({ row: { @@ -67,7 +70,8 @@ const getMultiSelectMenuOptions = (selectedRowsList: RowProp[]) => { .filter(value => value.row.depth === 1) .map(value => value.row.values.id) - const hideRemoveOption = removableRowIds.length !== selectedRowsList.length + const hideRemoveOption = + removableRowIds.length !== selectedRowsList.length || hasRunningExperiment const toggleStarOption = (ids: string[], label: string) => experimentMenuOption( @@ -92,6 +96,46 @@ const getMultiSelectMenuOptions = (selectedRowsList: RowProp[]) => { MessageFromWebviewType.REMOVE_EXPERIMENT, hideRemoveOption, true + ), + { + divider: true, + id: 'clear-selection', + keyboardShortcut: 'Esc', + label: 'Clear row selection' + } + ] +} + +const getRunResumeOptions = ( + withId: ( + label: string, + type: MessageFromWebviewType, + hidden?: boolean, + divider?: boolean + ) => MessagesMenuOptionProps, + isWorkspace: boolean, + projectHasCheckpoints: boolean, + hideVaryAndRun: boolean, + depth: number +) => { + const isNotCheckpoint = depth <= 1 || isWorkspace + + return [ + withId( + 'Modify, Reset and Run', + MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_RESET_AND_RUN, + !isNotCheckpoint || !projectHasCheckpoints + ), + withId( + projectHasCheckpoints ? 'Modify and Resume' : 'Modify and Run', + MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_AND_RUN, + !isNotCheckpoint, + !hideVaryAndRun + ), + withId( + 'Modify and Queue', + MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_AND_QUEUE, + !isNotCheckpoint ) ] } @@ -100,53 +144,51 @@ const getSingleSelectMenuOptions = ( id: string, isWorkspace: boolean, projectHasCheckpoints: boolean, + hasRunningExperiment: boolean, depth: number, queued?: boolean, starred?: boolean ) => { - const isNotCheckpoint = depth <= 1 || isWorkspace - const canApplyOrCreateBranch = queued || isWorkspace || depth <= 0 + const hideApplyAndCreateBranch = queued || isWorkspace || depth <= 0 const withId = ( label: string, type: MessageFromWebviewType, hidden?: boolean, divider?: boolean - ) => experimentMenuOption(id, label, type, hidden, divider) + ) => + experimentMenuOption( + id, + label, + type, + hidden || hasRunningExperiment, + divider + ) return [ withId( 'Apply to Workspace', MessageFromWebviewType.APPLY_EXPERIMENT_TO_WORKSPACE, - canApplyOrCreateBranch + hideApplyAndCreateBranch ), withId( 'Create new Branch', MessageFromWebviewType.CREATE_BRANCH_FROM_EXPERIMENT, - canApplyOrCreateBranch - ), - withId( - projectHasCheckpoints ? 'Modify and Resume' : 'Modify and Run', - MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_AND_RUN, - !isNotCheckpoint, - !canApplyOrCreateBranch + hideApplyAndCreateBranch ), - withId( - 'Modify, Reset and Run', - MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_RESET_AND_RUN, - !isNotCheckpoint || !projectHasCheckpoints - ), - withId( - 'Modify and Queue', - MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_AND_QUEUE, - !isNotCheckpoint + ...getRunResumeOptions( + withId, + isWorkspace, + projectHasCheckpoints, + hideApplyAndCreateBranch, + depth ), experimentMenuOption( [id], starred ? 'Unstar Experiment' : 'Star Experiment', MessageFromWebviewType.TOGGLE_EXPERIMENT_STAR, isWorkspace, - true + !hasRunningExperiment ), withId( 'Remove', @@ -161,6 +203,7 @@ const getContextMenuOptions = ( id: string, isWorkspace: boolean, projectHasCheckpoints: boolean, + hasRunningExperiment: boolean, depth: number, selectedRows: Record, queued?: boolean, @@ -173,12 +216,13 @@ const getContextMenuOptions = ( return cond( isFromSelection && selectedRowsList.length > 1, - () => getMultiSelectMenuOptions(selectedRowsList), + () => getMultiSelectMenuOptions(selectedRowsList, hasRunningExperiment), () => getSingleSelectMenuOptions( id, isWorkspace, projectHasCheckpoints, + hasRunningExperiment, depth, queued, starred @@ -187,6 +231,7 @@ const getContextMenuOptions = ( } export const RowContextMenu: React.FC = ({ + hasRunningExperiment = false, projectHasCheckpoints = false, row: { original: { queued, starred }, @@ -204,6 +249,7 @@ export const RowContextMenu: React.FC = ({ id, isWorkspace, projectHasCheckpoints, + hasRunningExperiment, depth, selectedRows, queued, @@ -216,7 +262,8 @@ export const RowContextMenu: React.FC = ({ depth, id, projectHasCheckpoints, - selectedRows + selectedRows, + hasRunningExperiment ]) return ( @@ -267,6 +314,7 @@ export const RowContent: React.FC< changes, contextMenuDisabled, projectHasCheckpoints, + hasRunningExperiment, batchRowSelection }): JSX.Element => { const { @@ -319,6 +367,7 @@ export const RowContent: React.FC< } > diff --git a/webview/src/experiments/components/table/RowSelectionContext.tsx b/webview/src/experiments/components/table/RowSelectionContext.tsx index 1f7f87a230..d6f6517c72 100644 --- a/webview/src/experiments/components/table/RowSelectionContext.tsx +++ b/webview/src/experiments/components/table/RowSelectionContext.tsx @@ -3,8 +3,9 @@ import { RowProp } from './interfaces' export interface RowSelectionContextValue { selectedRows: Record - toggleRowSelected: ((row: RowProp) => void) | undefined - batchSelection: ((batch: RowProp[]) => void) | undefined + lastSelectedRow?: RowProp + toggleRowSelected?: (row: RowProp) => void + batchSelection?: (batch: RowProp[]) => void clearSelectedRows: (() => void) | undefined } @@ -22,6 +23,8 @@ export const RowSelectionProvider: React.FC<{ children: React.ReactNode }> = ({ Record >({}) + const [selectionHistory, setSelectionHistory] = useState([]) + const toggleRowSelected = (rowProp: RowProp) => { const { row: { @@ -32,6 +35,7 @@ export const RowSelectionProvider: React.FC<{ children: React.ReactNode }> = ({ ...selectedRows, [id]: selectedRows[id] ? undefined : rowProp }) + setSelectionHistory([id, ...selectionHistory]) } const batchSelection = (batch: RowProp[]) => { @@ -47,17 +51,35 @@ export const RowSelectionProvider: React.FC<{ children: React.ReactNode }> = ({ } setSelectedRows(selectedRowsCopy) + setSelectionHistory([ + ...batch.map( + ({ + row: { + values: { id } + } + }) => id + ), + ...selectionHistory + ]) } const clearSelectedRows = () => { setSelectedRows({}) + setSelectionHistory([]) } + const lastSelectedRow = React.useMemo(() => { + const lastSelectedId = selectionHistory.find(id => selectedRows[id]) ?? '' + + return selectedRows[lastSelectedId] + }, [selectedRows, selectionHistory]) + return ( { instance.prepareRow(row) @@ -23,6 +24,7 @@ export const NestedRow: React.FC< className={styles.nestedRow} contextMenuDisabled={contextMenuDisabled} projectHasCheckpoints={projectHasCheckpoints} + hasRunningExperiment={hasRunningExperiment} batchRowSelection={batchRowSelection} /> ) @@ -35,6 +37,7 @@ export const ExperimentGroup: React.FC< instance, contextMenuDisabled, projectHasCheckpoints, + hasRunningExperiment, batchRowSelection }) => { instance.prepareRow(row) @@ -50,6 +53,7 @@ export const ExperimentGroup: React.FC< instance={instance} contextMenuDisabled={contextMenuDisabled} projectHasCheckpoints={projectHasCheckpoints} + hasRunningExperiment={hasRunningExperiment} batchRowSelection={batchRowSelection} /> {row.isExpanded && @@ -60,6 +64,7 @@ export const ExperimentGroup: React.FC< key={row.id} contextMenuDisabled={contextMenuDisabled} projectHasCheckpoints={projectHasCheckpoints} + hasRunningExperiment={hasRunningExperiment} batchRowSelection={batchRowSelection} /> ))} @@ -75,6 +80,7 @@ export const TableBody: React.FC< changes, contextMenuDisabled, projectHasCheckpoints, + hasRunningExperiment, batchRowSelection }) => { instance.prepareRow(row) @@ -93,6 +99,7 @@ export const TableBody: React.FC< ))} @@ -127,7 +135,7 @@ export const Table: React.FC = ({ filteredCounts } = tableData - const { clearSelectedRows, batchSelection, selectedRows } = + const { clearSelectedRows, batchSelection, lastSelectedRow } = React.useContext(RowSelectionContext) const tableRef = useRef(null) @@ -139,29 +147,47 @@ export const Table: React.FC = ({ useClickOutside(tableRef, clickOutsideHandler) const batchRowSelection = React.useCallback( - ({ row: { flatIndex } }: RowProp) => { - const firstSelection = flatRows.find( - ({ values: { id } }) => selectedRows[id] - ) + ({ row: { id } }: RowProp) => { + const lastSelectedRowId = lastSelectedRow?.row.id ?? '' + const lastIndex = + flatRows.findIndex(flatRow => flatRow.id === lastSelectedRowId) || 1 + const selectedIndex = + flatRows.findIndex(flatRow => flatRow.id === id) || 1 + const rangeStart = Math.min(lastIndex, selectedIndex) + const rangeEnd = Math.max(lastIndex, selectedIndex) - const firstIndex = firstSelection?.flatIndex || 1 + const collapsedIds = flatRows + .filter(flatRow => !flatRow.isExpanded) + .map(flatRow => flatRow.id) - if (flatIndex >= firstIndex) { - const batch = flatRows - .filter( - row => row.flatIndex > firstIndex && row.flatIndex <= flatIndex - ) - .map(row => ({ row })) + const batch = flatRows + .slice(rangeStart, rangeEnd + 1) + .filter( + flatRow => + !collapsedIds.some(collapsedId => + flatRow.id.startsWith(`${collapsedId}.`) + ) + ) + .map(row => ({ row })) - batchSelection?.(batch) - } + batchSelection?.(batch) }, - [selectedRows, flatRows, batchSelection] + [flatRows, batchSelection, lastSelectedRow] ) return (
-
+
{ + if (e.key === 'Escape') { + clearSelectedRows?.() + } + }} + > = ({ instance={instance} key={row.id} changes={changes} - contextMenuDisabled={hasRunningExperiment} + hasRunningExperiment={hasRunningExperiment} projectHasCheckpoints={hasCheckpoints} batchRowSelection={batchRowSelection} /> diff --git a/webview/src/experiments/components/table/interfaces.ts b/webview/src/experiments/components/table/interfaces.ts index 02320bae31..8dc2626d3e 100644 --- a/webview/src/experiments/components/table/interfaces.ts +++ b/webview/src/experiments/components/table/interfaces.ts @@ -16,6 +16,7 @@ export interface WithChanges { export interface RowProp { row: Row contextMenuDisabled?: boolean + hasRunningExperiment?: boolean projectHasCheckpoints?: boolean } diff --git a/webview/src/shared/components/messagesMenu/MessagesMenuOption.tsx b/webview/src/shared/components/messagesMenu/MessagesMenuOption.tsx index ff4ff5f1d0..f8cf4087b2 100644 --- a/webview/src/shared/components/messagesMenu/MessagesMenuOption.tsx +++ b/webview/src/shared/components/messagesMenu/MessagesMenuOption.tsx @@ -7,16 +7,17 @@ import { sendMessage } from '../../vscode' export interface MessagesMenuOptionProps { id: string label: string - message: MessageFromWebview + message?: MessageFromWebview hidden?: boolean divider?: boolean + keyboardShortcut?: string } export const MessagesMenuOption: React.FC< MessagesMenuOptionProps & { onOptionSelected?: () => void } -> = ({ label, message, divider, onOptionSelected }) => { +> = ({ label, message, divider, onOptionSelected, keyboardShortcut }) => { const sendTheMessage = () => { - sendMessage(message) + !!message && sendMessage(message) onOptionSelected?.() } @@ -44,6 +45,7 @@ export const MessagesMenuOption: React.FC< > {label}
+ {keyboardShortcut &&
{keyboardShortcut}
}
)