Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add copy sha and name to experiment table row context menu #4544

Merged
merged 2 commits into from
Aug 21, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 3 additions & 0 deletions extension/src/__mocks__/vscode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ import { join } from 'path'
import { URI, Utils } from 'vscode-uri'

export const commands = jest.fn()
export const env = {
clipboard: jest.fn()
}
export const EventEmitter = jest.fn()
export const Extension = jest.fn()
export const extensions = jest.fn()
Expand Down
13 changes: 13 additions & 0 deletions extension/src/experiments/webview/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { Pipeline } from '../../pipeline'
import { collectColumnsWithChangedValues } from '../columns/collect'
import { ColumnLike } from '../columns/like'
import { getFilterId } from '../model/filterBy'
import { writeToClipboard } from '../../vscode/clipboard'

export class WebviewMessages {
private readonly dvcRoot: string
Expand Down Expand Up @@ -227,6 +228,9 @@ export class WebviewMessages {
case MessageFromWebviewType.TOGGLE_SHOW_ONLY_CHANGED:
return this.toggleShowOnlyChanged()

case MessageFromWebviewType.COPY_TO_CLIPBOARD:
return this.copyToClipboard(message.payload)

default:
Logger.error(`Unexpected message: ${JSON.stringify(message)}`)
}
Expand Down Expand Up @@ -571,4 +575,13 @@ export class WebviewMessages {
private showPlots() {
return commands.executeCommand(RegisteredCommands.PLOTS_SHOW, this.dvcRoot)
}

private async copyToClipboard(text: string) {
await writeToClipboard(text)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be copying the full sha instead of the short version? When I see these context options, my first assumption is I'm copying the full sha, but on the other hand, the shortened version is probably all that's needed for most use cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The short sha should work for now.

void sendTelemetryEvent(
EventName.VIEWS_EXPERIMENTS_TABLE_COPY_TO_CLIPBOARD,
undefined,
undefined
)
}
}
3 changes: 3 additions & 0 deletions extension/src/telemetry/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ export const EventName = Object.assign(
VIEWS_EXPERIMENTS_TABLE_CLOSED: 'views.experimentsTable.closed',
VIEWS_EXPERIMENTS_TABLE_COLUMNS_REORDERED:
'views.experimentsTable.columnsReordered',
VIEWS_EXPERIMENTS_TABLE_COPY_TO_CLIPBOARD:
'views.experimentsTable.copyToClipboard',
VIEWS_EXPERIMENTS_TABLE_CREATED: 'views.experimentsTable.created',
VIEWS_EXPERIMENTS_TABLE_EXPERIMENT_STARS_TOGGLE:
'views.experimentTable.toggleStars',
Expand Down Expand Up @@ -257,6 +259,7 @@ export interface IEventNamePropertyMapping {
path: string
}
[EventName.VIEWS_EXPERIMENTS_TABLE_RESET_COMMITS]: undefined
[EventName.VIEWS_EXPERIMENTS_TABLE_COPY_TO_CLIPBOARD]: undefined
[EventName.VIEWS_EXPERIMENTS_TABLE_CREATED]: undefined
[EventName.VIEWS_EXPERIMENTS_TABLE_FILTER_COLUMN]: undefined
[EventName.VIEWS_EXPERIMENTS_TABLE_REMOVE_COLUMN_FILTER]: undefined
Expand Down
27 changes: 27 additions & 0 deletions extension/src/test/suite/experiments/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ import { Response } from '../../../vscode/response'
import { MAX_SELECTED_EXPERIMENTS } from '../../../experiments/model/status'
import { Pipeline } from '../../../pipeline'
import { ColumnLike } from '../../../experiments/columns/like'
import * as Clipboard from '../../../vscode/clipboard'

const { openFileInEditor } = FileSystem

Expand Down Expand Up @@ -1575,6 +1576,32 @@ suite('Experiments Test Suite', () => {
expect(mockUpdateExperimentsData).to.be.calledOnce
}).timeout(WEBVIEW_TEST_TIMEOUT)

it('should be able to handle a message to copy an experiment name', async () => {
const { webview } = await buildExperimentsWebview({
disposer: disposable
})
const mockMessageReceived = getMessageReceivedEmitter(webview)
const mockExperimentId = 'mock-experiment-id'

const mockCopyToClipboard = stub(Clipboard, 'writeToClipboard')
const copyCalled = new Promise(resolve =>
mockCopyToClipboard.callsFake(() => {
resolve(undefined)
return Promise.resolve()
})
)

mockMessageReceived.fire({
payload: mockExperimentId,
type: MessageFromWebviewType.COPY_TO_CLIPBOARD
})

await copyCalled

expect(mockCopyToClipboard).to.be.calledOnce
expect(mockCopyToClipboard).to.be.calledWithExactly(mockExperimentId)
}).timeout(WEBVIEW_TEST_TIMEOUT)

it('should not update the selected branches when the user closes the select branches quick pick', async () => {
const {
experimentsModel,
Expand Down
9 changes: 9 additions & 0 deletions extension/src/vscode/clipboard.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { env } from 'vscode'
import { Toast } from './toast'

const { clipboard } = env

export const writeToClipboard = async (text: string): Promise<void> => {
await clipboard.writeText(text)
void Toast.infoWithOptions(`${text} copied to clipboard`)
}
5 changes: 5 additions & 0 deletions extension/src/webview/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export enum MessageFromWebviewType {
ADD_STARRED_EXPERIMENT_FILTER = 'add-starred-experiment-filter',
ADD_CUSTOM_PLOT = 'add-custom-plot',
CREATE_BRANCH_FROM_EXPERIMENT = 'create-branch-from-experiment',
COPY_TO_CLIPBOARD = 'copy-to-clipboard',
EXPORT_PLOT_DATA_AS_JSON = 'export-plot-data-as-json',
EXPORT_PLOT_DATA_AS_CSV = 'export-plot-data-as-csv',
EXPORT_PLOT_DATA_AS_TSV = 'export-plot-data-as-tsv',
Expand Down Expand Up @@ -108,6 +109,10 @@ export type MessageFromWebview =
| {
type: MessageFromWebviewType.ADD_CUSTOM_PLOT
}
| {
type: MessageFromWebviewType.COPY_TO_CLIPBOARD
payload: string
}
| {
type: MessageFromWebviewType.EXPORT_PLOT_DATA_AS_JSON
payload: string
Expand Down
22 changes: 16 additions & 6 deletions webview/src/experiments/components/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1036,6 +1036,7 @@ describe('App', () => {
expect(getEnabledOptions()).toStrictEqual([
'Apply to Workspace',
'Create new Branch',
'Copy Sha',
'Star'
])
})
Expand All @@ -1049,6 +1050,7 @@ describe('App', () => {
expect(getEnabledOptions()).toStrictEqual([
'Apply to Workspace',
'Create new Branch',
'Copy Sha',
'Star'
])
})
Expand All @@ -1062,6 +1064,8 @@ describe('App', () => {
expect(getEnabledOptions()).toStrictEqual([
'Apply to Workspace',
'Create new Branch',
'Copy Sha',
'Copy Experiment Name',
'Push',
'Star',
'Remove'
Expand All @@ -1080,7 +1084,13 @@ describe('App', () => {
const target = screen.getByText('[exp-e7a67]')
fireEvent.contextMenu(target, { bubbles: true })

expect(getEnabledOptions()).toStrictEqual(['Show Logs', 'Star', 'Stop'])
expect(getEnabledOptions()).toStrictEqual([
'Show Logs',
'Copy Sha',
'Copy Experiment Name',
'Star',
'Stop'
])

fireEvent.click(screen.getAllByRole('menuitem')[0], {
bubbles: true,
Expand Down Expand Up @@ -1108,7 +1118,7 @@ describe('App', () => {
expect(disabledMenuItem).toBeDefined()

disabledMenuItem && fireEvent.click(disabledMenuItem, { bubbles: true })
expect(screen.queryAllByRole('menuitem')).toHaveLength(10)
expect(screen.queryAllByRole('menuitem')).toHaveLength(12)
})

it('should be removed with a left click', () => {
Expand All @@ -1118,7 +1128,7 @@ describe('App', () => {
fireEvent.contextMenu(row, { bubbles: true })

advanceTimersByTime(100)
expect(screen.getAllByRole('menuitem')).toHaveLength(10)
expect(screen.getAllByRole('menuitem')).toHaveLength(12)

fireEvent.click(row, {
bubbles: true
Expand All @@ -1135,7 +1145,7 @@ describe('App', () => {
fireEvent.contextMenu(row, { bubbles: true })

advanceTimersByTime(100)
expect(screen.getAllByRole('menuitem')).toHaveLength(10)
expect(screen.getAllByRole('menuitem')).toHaveLength(12)

const commit = getRow('main')
fireEvent.click(commit, { bubbles: true })
Expand All @@ -1150,13 +1160,13 @@ describe('App', () => {
fireEvent.contextMenu(row, { bubbles: true })

advanceTimersByTime(100)
expect(screen.queryAllByRole('menuitem')).toHaveLength(10)
expect(screen.queryAllByRole('menuitem')).toHaveLength(12)

fireEvent.contextMenu(within(row).getByText('[exp-e7a67]'), {
bubbles: true
})
advanceTimersByTime(200)
expect(screen.queryAllByRole('menuitem')).toHaveLength(10)
expect(screen.queryAllByRole('menuitem')).toHaveLength(12)
})

it('should enable the remove option for an experiment', () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import React, { useMemo } from 'react'
import { useDispatch, useSelector } from 'react-redux'
import { MessageFromWebviewType } from 'dvc/src/webview/contract'
import {
MessageFromWebview,
MessageFromWebviewType
} from 'dvc/src/webview/contract'
import {
WORKSPACE_BRANCH,
ExecutorStatus,
Expand Down Expand Up @@ -35,8 +38,8 @@ const experimentMenuOption = (
message: {
payload,
type
}
} as MessagesMenuOptionProps
} as MessageFromWebview
}
}

const collectIdByStarred = (
Expand Down Expand Up @@ -212,6 +215,7 @@ const getRunResumeOptions = (

const getSingleSelectMenuOptions = (
id: string,
sha: string,
isWorkspace: boolean,
projectHasCheckpoints: boolean,
hasRunningWorkspaceExperiment: boolean,
Expand Down Expand Up @@ -263,6 +267,24 @@ const getSingleSelectMenuOptions = (
'Create new Branch',
MessageFromWebviewType.CREATE_BRANCH_FROM_EXPERIMENT
),
{
disabled: isWorkspace,
id: MessageFromWebviewType.COPY_TO_CLIPBOARD,
label: 'Copy Sha',
message: {
payload: sha,
type: MessageFromWebviewType.COPY_TO_CLIPBOARD
} as MessageFromWebview
},
{
disabled: isWorkspace || depth <= 0,
id: MessageFromWebviewType.COPY_TO_CLIPBOARD,
label: 'Copy Experiment Name',
message: {
payload: id,
type: MessageFromWebviewType.COPY_TO_CLIPBOARD
} as MessageFromWebview
},
experimentMenuOption(
[id],
'Push',
Expand Down Expand Up @@ -302,6 +324,7 @@ const getSingleSelectMenuOptions = (

const getContextMenuOptions = (
id: string,
sha: string,
branch: string | undefined | typeof WORKSPACE_BRANCH,
isWorkspace: boolean,
projectHasCheckpoints: boolean,
Expand All @@ -327,6 +350,7 @@ const getContextMenuOptions = (
() =>
getSingleSelectMenuOptions(
id,
sha,
isWorkspace,
projectHasCheckpoints,
hasRunningWorkspaceExperiment,
Expand All @@ -340,7 +364,7 @@ const getContextMenuOptions = (

export const RowContextMenu: React.FC<RowProp> = ({
row: {
original: { branch, executorStatus, starred, id, executor },
original: { branch, executorStatus, starred, id, executor, label },
depth
}
}) => {
Expand All @@ -358,6 +382,7 @@ export const RowContextMenu: React.FC<RowProp> = ({
const contextMenuOptions = useMemo(() => {
return getContextMenuOptions(
id,
label,
branch,
isWorkspace,
projectHasCheckpoints,
Expand All @@ -376,6 +401,7 @@ export const RowContextMenu: React.FC<RowProp> = ({
isWorkspace,
depth,
id,
label,
projectHasCheckpoints,
selectedRows,
hasRunningWorkspaceExperiment
Expand Down