diff --git a/extension/src/plots/errors/model.ts b/extension/src/plots/errors/model.ts index f6d6d66e9a..06a0ce16e8 100644 --- a/extension/src/plots/errors/model.ts +++ b/extension/src/plots/errors/model.ts @@ -10,6 +10,7 @@ import { Disposable } from '../../class/dispose' import { DvcError, PlotsOutputOrError } from '../../cli/dvc/contract' import { isDvcError } from '../../cli/dvc/reader' import { getCliErrorLabel } from '../../tree' +import { PlotErrors } from '../webview/contract' export class ErrorsModel extends Disposable { private readonly dvcRoot: string @@ -66,6 +67,17 @@ export class ErrorsModel extends Disposable { return [...acc] } + public getErrorsByPath(paths: string[], selectedRevisions: string[]) { + const errors: PlotErrors = [] + for (const path of paths) { + const pathErrors = this.getPathErrors(path, selectedRevisions) + if (pathErrors) { + errors.push({ path, revs: pathErrors }) + } + } + return errors + } + public hasCliError() { return !!this.getCliError() } diff --git a/extension/src/plots/paths/model.ts b/extension/src/plots/paths/model.ts index bc0542fd5e..7d99b5dd17 100644 --- a/extension/src/plots/paths/model.ts +++ b/extension/src/plots/paths/model.ts @@ -142,6 +142,22 @@ export class PathsModel extends PathSelectionModel { return false } + public getSelectedPlotPaths() { + const revisionPaths = this.data.filter(element => + this.hasRevisions(element) + ) + + const paths: string[] = [] + + for (const { path } of revisionPaths) { + if (this.status[path] === Status.SELECTED) { + paths.push(path) + } + } + + return paths + } + public getTemplateOrder(): TemplateOrder { return collectTemplateOrder( this.getPathsByType(PathType.TEMPLATE_SINGLE), diff --git a/extension/src/plots/webview/contract.ts b/extension/src/plots/webview/contract.ts index 1c8293433f..4a675cd1c3 100644 --- a/extension/src/plots/webview/contract.ts +++ b/extension/src/plots/webview/contract.ts @@ -163,12 +163,18 @@ export type ComparisonPlot = { imgs: ComparisonPlotImg[] } +export type PlotErrors = { + path: string + revs: { rev: string; msg: string }[] +}[] + export enum PlotsDataKeys { COMPARISON = 'comparison', CLI_ERROR = 'cliError', CUSTOM = 'custom', HAS_UNSELECTED_PLOTS = 'hasUnselectedPlots', HAS_PLOTS = 'hasPlots', + PLOT_ERRORS = 'plotErrors', SELECTED_REVISIONS = 'selectedRevisions', TEMPLATE = 'template', SECTION_COLLAPSED = 'sectionCollapsed', @@ -188,6 +194,7 @@ export type PlotsData = [PlotsDataKeys.SECTION_COLLAPSED]?: SectionCollapsed [PlotsDataKeys.SHOW_TOO_MANY_TEMPLATE_PLOTS]?: boolean [PlotsDataKeys.SHOW_TOO_MANY_COMPARISON_IMAGES]?: boolean + [PlotsDataKeys.PLOT_ERRORS]?: PlotErrors } | undefined diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts index 6a49c47706..22e8b727e1 100644 --- a/extension/src/plots/webview/messages.ts +++ b/extension/src/plots/webview/messages.ts @@ -174,7 +174,8 @@ export class WebviewMessages { hasPlots, hasUnselectedPlots, sectionCollapsed, - template + template, + plotErrors ] = await Promise.all([ this.errors.getCliError()?.error || null, this.getComparisonPlots(), @@ -182,7 +183,11 @@ export class WebviewMessages { !!this.paths.hasPaths(), this.paths.getHasUnselectedPlots(), this.plots.getSectionCollapsed(), - this.getTemplatePlots(selectedRevisions) + this.getTemplatePlots(selectedRevisions), + this.errors.getErrorsByPath( + this.paths.getSelectedPlotPaths(), + this.plots.getSelectedRevisionIds() + ) ]) const shouldShowTooManyTemplatePlotsMessage = this.shouldShowTooManyPlotsMessage([ @@ -198,6 +203,7 @@ export class WebviewMessages { custom, hasPlots, hasUnselectedPlots, + plotErrors, sectionCollapsed, selectedRevisions, shouldShowTooManyComparisonImagesMessage, diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 78a7741e55..042d62f652 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -1104,6 +1104,66 @@ suite('Plots Test Suite', () => { ).not.to.contain(brokenExp) }) + it('should send plot errors to the webview', async () => { + const accPngPath = join('plots', 'acc.png') + const accPng = [ + ...plotsDiffFixture.data[join('plots', 'acc.png')] + ] as ImagePlot[] + const lossTsvPath = join('logs', 'loss.tsv') + const lossTsv = [ + ...plotsDiffFixture.data[lossTsvPath] + ] as TemplatePlotOutput[] + + const plotsDiffOutput = { + data: { + [accPngPath]: accPng, + [lossTsvPath]: lossTsv + }, + errors: [ + { + msg: 'File not found', + name: accPngPath, + rev: 'workspace', + type: 'FileNotFoundError' + }, + { + msg: 'Could not find provided field', + name: lossTsvPath, + rev: 'workspace', + type: 'FieldNotFoundError' + }, + { + msg: 'Could not find provided field', + name: lossTsvPath, + rev: 'main', + type: 'FieldNotFoundError' + } + ] + } + const { messageSpy } = await buildPlotsWebview({ + disposer: disposable, + plotsDiff: plotsDiffOutput + }) + + const expectedPlotsData: TPlotsData = { + plotErrors: [ + { + path: accPngPath, + revs: [{ msg: 'File not found', rev: 'workspace' }] + }, + { + path: lossTsvPath, + revs: [ + { msg: 'Could not find provided field', rev: 'workspace' }, + { msg: 'Could not find provided field', rev: 'main' } + ] + } + ] + } + + expect(messageSpy).to.be.calledWithMatch(expectedPlotsData) + }).timeout(WEBVIEW_TEST_TIMEOUT) + it('should handle a toggle experiment message from the webview', async () => { const { experiments, mockMessageReceived } = await buildPlotsWebview({ disposer: disposable, diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index 2544c369ac..eada042721 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/webview/src/plots/components/App.test.tsx @@ -2796,6 +2796,49 @@ describe('App', () => { }) expect(screen.queryByText('!')).not.toBeInTheDocument() }) + + it('should show an error banner when there are plot errors found', () => { + renderAppWithOptionalData({ + comparison: comparisonTableFixture, + plotErrors: [ + { path: 'image-path', revs: [{ msg: 'error', rev: 'main' }] } + ] + }) + + expect(screen.getByText('Show error')).toBeInTheDocument() + + sendSetDataMessage({ + plotErrors: [ + { path: 'image-path', revs: [{ msg: 'error', rev: 'main' }] }, + { path: 'second-image-path', revs: [{ msg: 'error', rev: 'main' }] } + ] + }) + + expect(screen.getByText('Show 2 errors')).toBeInTheDocument() + }) + + it('should show a button that opens an error modal when there are plot errors found', () => { + renderAppWithOptionalData({ + comparison: comparisonTableFixture, + plotErrors: [ + { path: 'image-path', revs: [{ msg: 'error', rev: 'main' }] } + ] + }) + + const showErrorsBtn = screen.getByText('Show error') + + expect(showErrorsBtn).toBeInTheDocument() + + fireEvent.click(showErrorsBtn) + + expect(screen.getByTestId('modal')).toBeInTheDocument() + + const modalContents = screen.getByTestId('errors-modal') + + expect(within(modalContents).getByText('image-path')).toBeInTheDocument() + expect(within(modalContents).getByText('main')).toBeInTheDocument() + expect(within(modalContents).getByText('error')).toBeInTheDocument() + }) }) describe('Vega panels', () => { diff --git a/webview/src/plots/components/App.tsx b/webview/src/plots/components/App.tsx index 5c021c21d6..cfb94cdaac 100644 --- a/webview/src/plots/components/App.tsx +++ b/webview/src/plots/components/App.tsx @@ -30,6 +30,7 @@ import { updateCliError, updateHasPlots, updateHasUnselectedPlots, + updatePlotErrors, updateSelectedRevisions } from './webviewSlice' import { PlotsDispatch } from '../store' @@ -81,6 +82,9 @@ export const feedStore = ( case PlotsDataKeys.HAS_UNSELECTED_PLOTS: dispatch(updateHasUnselectedPlots(!!data.data[key])) continue + case PlotsDataKeys.PLOT_ERRORS: + dispatch(updatePlotErrors(data.data[key])) + continue case PlotsDataKeys.SELECTED_REVISIONS: dispatch(updateSelectedRevisions(data.data[key])) continue diff --git a/webview/src/plots/components/ErrorsModal.tsx b/webview/src/plots/components/ErrorsModal.tsx new file mode 100644 index 0000000000..7a20a818c6 --- /dev/null +++ b/webview/src/plots/components/ErrorsModal.tsx @@ -0,0 +1,39 @@ +import React from 'react' +import { useSelector } from 'react-redux' +import styles from './styles.module.scss' +import { Error } from '../../shared/components/icons' +import { PlotsState } from '../store' +import { useModalOpenClass } from '../hooks/useModalOpenClass' + +export const ErrorsModal: React.FC = () => { + const errors = useSelector((state: PlotsState) => state.webview.plotErrors) + useModalOpenClass() + + return ( +
+

+ + Errors +

+ + + {errors.map(({ path, revs }) => ( + + + + + {revs.map(({ rev, msg }) => ( + + + + + ))} + + ))} + +
+ {path} +
{rev}{msg}
+
+ ) +} diff --git a/webview/src/plots/components/PlotsContent.tsx b/webview/src/plots/components/PlotsContent.tsx index a2c9137ded..3136ba5f57 100644 --- a/webview/src/plots/components/PlotsContent.tsx +++ b/webview/src/plots/components/PlotsContent.tsx @@ -3,11 +3,16 @@ import { useSelector, useDispatch } from 'react-redux' import { ErrorState } from './emptyState/ErrorState' import { GetStarted } from './emptyState/GetStarted' import { ZoomedInPlot } from './ZoomedInPlot' +import { ErrorsModal } from './ErrorsModal' import { CustomPlotsWrapper } from './customPlots/CustomPlotsWrapper' import { TemplatePlotsWrapper } from './templatePlots/TemplatePlotsWrapper' import { ComparisonTableWrapper } from './comparisonTable/ComparisonTableWrapper' import { Ribbon } from './ribbon/Ribbon' -import { setMaxNbPlotsPerRow, setZoomedInPlot } from './webviewSlice' +import { + setMaxNbPlotsPerRow, + setZoomedInPlot, + setShowErrorsModal +} from './webviewSlice' import styles from './styles.module.scss' import { EmptyState } from '../../shared/components/emptyState/EmptyState' import { Modal } from '../../shared/components/modal/Modal' @@ -15,8 +20,14 @@ import { PlotsState } from '../store' export const PlotsContent = () => { const dispatch = useDispatch() - const { hasData, hasPlots, hasUnselectedPlots, zoomedInPlot, cliError } = - useSelector((state: PlotsState) => state.webview) + const { + hasData, + hasPlots, + hasUnselectedPlots, + zoomedInPlot, + cliError, + showErrorsModal + } = useSelector((state: PlotsState) => state.webview) const hasComparisonData = useSelector( (state: PlotsState) => state.comparison.hasData ) @@ -63,6 +74,16 @@ export const PlotsContent = () => { ) + const errorsModal = showErrorsModal && ( + { + dispatch(setShowErrorsModal(false)) + }} + > + + + ) + const hasCustomPlots = customPlotIds.length > 0 if (cliError) { @@ -93,6 +114,7 @@ export const PlotsContent = () => { {modal} + {errorsModal} ) } diff --git a/webview/src/plots/components/ZoomedInPlot.tsx b/webview/src/plots/components/ZoomedInPlot.tsx index 7d656b10bc..6f0e397e9b 100644 --- a/webview/src/plots/components/ZoomedInPlot.tsx +++ b/webview/src/plots/components/ZoomedInPlot.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useRef } from 'react' +import React, { useRef } from 'react' import { PlotsSection } from 'dvc/src/plots/webview/contract' import { View } from 'react-vega' import { ExtendedVegaLite } from './vegaLite/ExtendedVegaLite' @@ -15,6 +15,7 @@ import { exportPlotDataAsJson, exportPlotDataAsTsv } from '../util/messages' +import { useModalOpenClass } from '../hooks/useModalOpenClass' type ZoomedInPlotProps = { id: string @@ -43,15 +44,7 @@ export const ZoomedInPlot: React.FC = ({ openActionsMenu }: ZoomedInPlotProps) => { const zoomedInPlotRef = useRef(null) - - useEffect(() => { - const modalOpenClass = 'modalOpen' - document.body.classList.add(modalOpenClass) - - return () => { - document.body.classList.remove(modalOpenClass) - } - }, []) + useModalOpenClass() const onNewView = (view: View) => { const actions: HTMLDivElement | null | undefined = diff --git a/webview/src/plots/components/ribbon/Errors.tsx b/webview/src/plots/components/ribbon/Errors.tsx new file mode 100644 index 0000000000..4aac4facba --- /dev/null +++ b/webview/src/plots/components/ribbon/Errors.tsx @@ -0,0 +1,36 @@ +import React, { useState, useEffect } from 'react' +import { useSelector, useDispatch } from 'react-redux' +import styles from './styles.module.scss' +import { PlotsState } from '../../store' +import { Error } from '../../../shared/components/icons' +import { setShowErrorsModal } from '../webviewSlice' + +export const Errors: React.FC = () => { + const dispatch = useDispatch() + const [errorsTotal, setErrorsTotal] = useState(0) + const errors = useSelector((state: PlotsState) => state.webview.plotErrors) + + useEffect(() => { + let total = 0 + for (const { revs } of errors) { + total += revs.length + } + setErrorsTotal(total) + }, [errors]) + + if (errors.length === 0) { + return + } + + return ( +
  • + +
  • + ) +} diff --git a/webview/src/plots/components/ribbon/Ribbon.tsx b/webview/src/plots/components/ribbon/Ribbon.tsx index a174708aa2..2f254eeb99 100644 --- a/webview/src/plots/components/ribbon/Ribbon.tsx +++ b/webview/src/plots/components/ribbon/Ribbon.tsx @@ -5,6 +5,7 @@ import { useInView } from 'react-intersection-observer' import styles from './styles.module.scss' import { RibbonBlock } from './RibbonBlock' import { update } from './ribbonSlice' +import { Errors } from './Errors' import { IconButton } from '../../../shared/components/button/IconButton' import { PlotsState } from '../../store' import { Add, ListFilter, Refresh } from '../../../shared/components/icons' @@ -95,6 +96,7 @@ export const Ribbon: React.FC = () => { onClear={() => removeRevision(revision.id)} /> ))} + ) } diff --git a/webview/src/plots/components/ribbon/styles.module.scss b/webview/src/plots/components/ribbon/styles.module.scss index 164ba5d871..d6131d2f97 100644 --- a/webview/src/plots/components/ribbon/styles.module.scss +++ b/webview/src/plots/components/ribbon/styles.module.scss @@ -198,3 +198,30 @@ display: block; white-space: nowrap; } + +.errors { + margin: 5px 0 0; + color: $error-color; + border-top: 1px solid $error-color; + border-bottom: 1px solid $error-color; + display: flex; + width: 100%; +} + +.errorsIcon { + margin-right: 3px; + vertical-align: bottom; +} + +.errorsButton { + @extend %buttonAsLink; + + color: inherit; + font-weight: 600; + padding: 6px 0; + + &:hover { + color: inherit; + text-decoration: underline; + } +} diff --git a/webview/src/plots/components/styles.module.scss b/webview/src/plots/components/styles.module.scss index 528cea8555..f14e3fb077 100644 --- a/webview/src/plots/components/styles.module.scss +++ b/webview/src/plots/components/styles.module.scss @@ -236,6 +236,37 @@ $gap: 20px; } } +.errorsModal { + width: 80vw; + max-height: calc(80vh - 100px); + color: $error-color; +} + +.errorsModalTitle { + margin: 0; +} + +.errorsModalIcon { + margin-right: 3px; + vertical-align: text-bottom; + fill: $error-color; +} + +.errorsModalPlot { + text-align: left; + padding: 15px 0 0; + font-weight: 600; +} + +.errorsModalRev { + padding: 10px 20px 0 0; +} + +.errorsModalMsgs { + width: 100%; + padding-top: 10px; +} + .vegaCustomAction:hover { cursor: pointer; } diff --git a/webview/src/plots/components/webviewSlice.ts b/webview/src/plots/components/webviewSlice.ts index c67323b896..fd0b30f298 100644 --- a/webview/src/plots/components/webviewSlice.ts +++ b/webview/src/plots/components/webviewSlice.ts @@ -1,6 +1,10 @@ /* eslint-disable @typescript-eslint/no-unsafe-member-access */ import { createSlice, PayloadAction } from '@reduxjs/toolkit' -import { PlotsSection, Revision } from 'dvc/src/plots/webview/contract' +import { + PlotErrors, + PlotsSection, + Revision +} from 'dvc/src/plots/webview/contract' type ZoomedInPlotState = { section: PlotsSection @@ -8,6 +12,7 @@ type ZoomedInPlotState = { openActionsMenu?: boolean refresh?: boolean } + export interface WebviewState { cliError: string | undefined hasData: boolean @@ -16,6 +21,8 @@ export interface WebviewState { selectedRevisions: Revision[] zoomedInPlot: ZoomedInPlotState | undefined maxNbPlotsPerRow: number + plotErrors: PlotErrors + showErrorsModal: boolean } export const webviewInitialState: WebviewState = { @@ -24,7 +31,9 @@ export const webviewInitialState: WebviewState = { hasPlots: false, hasUnselectedPlots: false, maxNbPlotsPerRow: 4, + plotErrors: [], selectedRevisions: [], + showErrorsModal: false, zoomedInPlot: { id: '', section: PlotsSection.TEMPLATE_PLOTS @@ -46,6 +55,12 @@ export const webviewSlice = createSlice({ const maxWidth = action.payload state.maxNbPlotsPerRow = Math.floor(maxWidth / 300) }, + setShowErrorsModal: ( + state: { showErrorsModal: boolean }, + action: PayloadAction + ) => { + state.showErrorsModal = action.payload + }, setZoomedInPlot: ( // eslint-disable-next-line @typescript-eslint/no-explicit-any state: any, @@ -88,6 +103,12 @@ export const webviewSlice = createSlice({ ) => { state.hasUnselectedPlots = action.payload }, + updatePlotErrors: ( + state: { plotErrors: PlotErrors }, + action: PayloadAction + ) => { + state.plotErrors = action.payload || [] + }, updateSelectedRevisions: ( state: { selectedRevisions: Revision[] }, action: PayloadAction @@ -103,7 +124,9 @@ export const { updateHasPlots, updateHasUnselectedPlots, updateSelectedRevisions, + updatePlotErrors, setZoomedInPlot, + setShowErrorsModal, setMaxNbPlotsPerRow } = webviewSlice.actions diff --git a/webview/src/plots/hooks/useModalOpenClass.ts b/webview/src/plots/hooks/useModalOpenClass.ts new file mode 100644 index 0000000000..3e9a657e27 --- /dev/null +++ b/webview/src/plots/hooks/useModalOpenClass.ts @@ -0,0 +1,12 @@ +import { useEffect } from 'react' + +export const useModalOpenClass = () => { + useEffect(() => { + const modalOpenClass = 'modalOpen' + document.body.classList.add(modalOpenClass) + + return () => { + document.body.classList.remove(modalOpenClass) + } + }, []) +} diff --git a/webview/src/stories/Plots.stories.tsx b/webview/src/stories/Plots.stories.tsx index 34ee8bc843..30bd58a35e 100644 --- a/webview/src/stories/Plots.stories.tsx +++ b/webview/src/stories/Plots.stories.tsx @@ -50,18 +50,21 @@ const MockedState: React.FC<{ data: PlotsData; children: React.ReactNode }> = ({ return <>{children} } +const defaultPlotsData = { + cliError: null, + comparison: comparisonPlotsFixture, + custom: customPlotsFixture, + hasPlots: true, + hasUnselectedPlots: false, + plotErrors: [], + sectionCollapsed: DEFAULT_SECTION_COLLAPSED, + selectedRevisions: plotsRevisionsFixture, + template: templatePlotsFixture +} + export default { args: { - data: { - cliError: null, - comparison: comparisonPlotsFixture, - custom: customPlotsFixture, - hasPlots: true, - hasUnselectedPlots: false, - sectionCollapsed: DEFAULT_SECTION_COLLAPSED, - selectedRevisions: plotsRevisionsFixture, - template: templatePlotsFixture - } + data: defaultPlotsData }, component: Plots, title: 'Plots' @@ -83,6 +86,29 @@ const Template: StoryFn<{ export const WithData = Template.bind({}) WithData.parameters = CHROMATIC_VIEWPORTS_WITH_DELAY +export const WithPlotsErrors = Template.bind({}) +const errorMsg = 'Could not find provided field' +WithPlotsErrors.args = { + data: { + ...defaultPlotsData, + plotErrors: [ + { + path: 'dvc.yaml:Loss', + revs: [{ msg: errorMsg, rev: 'main' }] + } + ], + selectedRevisions: plotsRevisionsFixture.map(rev => { + if (rev.id === 'main') { + return { + ...rev, + errors: [errorMsg] + } + } + return rev + }) + } +} + export const WithCustomOnly = Template.bind({}) WithCustomOnly.args = { data: {