Skip to content

Commit

Permalink
Add plots error message below ribbon (#5165)
Browse files Browse the repository at this point in the history
  • Loading branch information
julieg18 authored Jan 9, 2024
1 parent 590aa4b commit 2da53ae
Show file tree
Hide file tree
Showing 17 changed files with 385 additions and 26 deletions.
12 changes: 12 additions & 0 deletions extension/src/plots/errors/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
}
Expand Down
16 changes: 16 additions & 0 deletions extension/src/plots/paths/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,22 @@ export class PathsModel extends PathSelectionModel<PlotPath> {
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),
Expand Down
7 changes: 7 additions & 0 deletions extension/src/plots/webview/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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

Expand Down
10 changes: 8 additions & 2 deletions extension/src/plots/webview/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,15 +174,20 @@ export class WebviewMessages {
hasPlots,
hasUnselectedPlots,
sectionCollapsed,
template
template,
plotErrors
] = await Promise.all([
this.errors.getCliError()?.error || null,
this.getComparisonPlots(),
this.getCustomPlots(),
!!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([
Expand All @@ -198,6 +203,7 @@ export class WebviewMessages {
custom,
hasPlots,
hasUnselectedPlots,
plotErrors,
sectionCollapsed,
selectedRevisions,
shouldShowTooManyComparisonImagesMessage,
Expand Down
60 changes: 60 additions & 0 deletions extension/src/test/suite/plots/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
43 changes: 43 additions & 0 deletions webview/src/plots/components/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
4 changes: 4 additions & 0 deletions webview/src/plots/components/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
updateCliError,
updateHasPlots,
updateHasUnselectedPlots,
updatePlotErrors,
updateSelectedRevisions
} from './webviewSlice'
import { PlotsDispatch } from '../store'
Expand Down Expand Up @@ -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
Expand Down
39 changes: 39 additions & 0 deletions webview/src/plots/components/ErrorsModal.tsx
Original file line number Diff line number Diff line change
@@ -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 (
<div className={styles.errorsModal} data-testid="errors-modal">
<h3 className={styles.errorsModalTitle}>
<Error className={styles.errorsModalIcon} width="20" height="20" />
Errors
</h3>
<table>
<tbody>
{errors.map(({ path, revs }) => (
<React.Fragment key={path}>
<tr>
<th colSpan={2} className={styles.errorsModalPlot}>
{path}
</th>
</tr>
{revs.map(({ rev, msg }) => (
<tr key={`${rev}-${msg}`}>
<td className={styles.errorsModalRev}>{rev}</td>
<td className={styles.errorsModalMsgs}>{msg}</td>
</tr>
))}
</React.Fragment>
))}
</tbody>
</table>
</div>
)
}
28 changes: 25 additions & 3 deletions webview/src/plots/components/PlotsContent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,31 @@ 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'
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
)
Expand Down Expand Up @@ -63,6 +74,16 @@ export const PlotsContent = () => {
</Modal>
)

const errorsModal = showErrorsModal && (
<Modal
onClose={() => {
dispatch(setShowErrorsModal(false))
}}
>
<ErrorsModal />
</Modal>
)

const hasCustomPlots = customPlotIds.length > 0

if (cliError) {
Expand Down Expand Up @@ -93,6 +114,7 @@ export const PlotsContent = () => {
<ComparisonTableWrapper />
<CustomPlotsWrapper />
{modal}
{errorsModal}
</div>
)
}
13 changes: 3 additions & 10 deletions webview/src/plots/components/ZoomedInPlot.tsx
Original file line number Diff line number Diff line change
@@ -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'
Expand All @@ -15,6 +15,7 @@ import {
exportPlotDataAsJson,
exportPlotDataAsTsv
} from '../util/messages'
import { useModalOpenClass } from '../hooks/useModalOpenClass'

type ZoomedInPlotProps = {
id: string
Expand Down Expand Up @@ -43,15 +44,7 @@ export const ZoomedInPlot: React.FC<ZoomedInPlotProps> = ({
openActionsMenu
}: ZoomedInPlotProps) => {
const zoomedInPlotRef = useRef<HTMLDivElement>(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 =
Expand Down
Loading

0 comments on commit 2da53ae

Please sign in to comment.