hoist revisions into top of webview message

iterative · Jun 6, 2022 · 80c64c4 · 80c64c4
1 parent ef364b6
commit 80c64c4
Show file tree

Hide file tree

Showing 13 changed files with 106 additions and 76 deletions.
diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts
@@ -172,10 +172,8 @@ export class Plots extends BaseRepository<TPlotsData> {
       comparison: this.getComparisonPlots(),
       hasPlots: !!this.paths?.hasPaths(),
       hasSelectedPlots: definedAndNonEmpty(this.paths?.getSelected()),
-      hasSelectedRevisions: definedAndNonEmpty(
-        this.plots?.getSelectedRevisionDetails()
-      ),
       sectionCollapsed: this.plots?.getSectionCollapsed(),
+      selectedRevisions: this.plots?.getSelectedRevisionDetails(),
       template: this.getTemplatePlots()
     })
   }
@@ -206,7 +204,6 @@ export class Plots extends BaseRepository<TPlotsData> {
       plots: comparison.map(({ path, revisions }) => {
         return { path, revisions: this.getRevisionsWithCorrectUrls(revisions) }
       }),
-      revisions: this.plots.getSelectedRevisionDetails(),
       sectionName: this.plots.getSectionName(Section.COMPARISON_TABLE),
       size: this.plots.getPlotSize(Section.COMPARISON_TABLE)
     }
@@ -317,7 +314,8 @@ export class Plots extends BaseRepository<TPlotsData> {
   private setComparisonOrder(order: string[]) {
     this.plots?.setComparisonOrder(order)
     this.webview?.show({
-      comparison: this.getComparisonPlots()
+      comparison: this.getComparisonPlots(),
+      selectedRevisions: this.plots?.getSelectedRevisionDetails()
     })
     sendTelemetryEvent(
       EventName.VIEWS_PLOTS_REVISIONS_REORDERED,

diff --git a/extension/src/plots/webview/contract.ts b/extension/src/plots/webview/contract.ts
@@ -51,7 +51,6 @@ export type Revision = {
 }
 
 export interface PlotsComparisonData {
-  revisions: Revision[]
   plots: ComparisonPlots
   sectionName: string
   size: PlotSize
@@ -137,7 +136,7 @@ export type PlotsData =
       checkpoint?: CheckpointPlotsData | null
       hasPlots?: boolean
       hasSelectedPlots?: boolean
-      hasSelectedRevisions?: boolean
+      selectedRevisions?: Revision[]
       template?: TemplatePlotsData | null
       sectionCollapsed?: SectionCollapsed
     }

diff --git a/extension/src/test/fixtures/plotsDiff/index.ts b/extension/src/test/fixtures/plotsDiff/index.ts
@@ -498,10 +498,42 @@ const extendedSpecs = (plotsOutput: TemplatePlots): TemplatePlotSection[] => {
   return [singleViewPlots, multiViewPlots]
 }
 
+export const getRevisions = () => {
+  const [workspace, main, _4fb124a, _42b8735, _1ba7bcd] = copyOriginalColors()
+  return [
+    {
+      id: 'workspace',
+      revision: 'workspace',
+      displayColor: workspace,
+      group: undefined
+    },
+    { id: 'main', revision: 'main', displayColor: main, group: undefined },
+    {
+      id: 'exp-e7a67',
+      revision: '4fb124a',
+      displayColor: _4fb124a,
+      group: '[exp-e7a67]'
+    },
+    {
+      id: 'test-branch',
+      revision: '42b8736',
+      displayColor: _42b8735,
+      group: '[test-branch]'
+    },
+    {
+      id: 'exp-83425',
+      revision: '1ba7bcd',
+      displayColor: _1ba7bcd,
+      group: '[exp-83425]'
+    }
+  ]
+}
+
 export const getMinimalWebviewMessage = () => ({
   plots: extendedSpecs(basicVega),
   sectionName: DEFAULT_SECTION_NAMES[Section.TEMPLATE_PLOTS],
-  size: PlotSize.REGULAR
+  size: PlotSize.REGULAR,
+  revisions: getRevisions()
 })
 
 export const getTemplateWebviewMessage = (): TemplatePlotsData => ({
@@ -540,37 +572,8 @@ export const getComparisonWebviewMessage = (
     plotAcc.push({ path, revisions: revisionsAcc })
   }
 
-  const [workspace, main, _4fb124a, _42b8735, _1ba7bcd] = copyOriginalColors()
-
   return {
     plots: plotAcc,
-    revisions: [
-      {
-        id: 'workspace',
-        revision: 'workspace',
-        displayColor: workspace,
-        group: undefined
-      },
-      { id: 'main', revision: 'main', displayColor: main, group: undefined },
-      {
-        id: 'exp-e7a67',
-        revision: '4fb124a',
-        displayColor: _4fb124a,
-        group: '[exp-e7a67]'
-      },
-      {
-        id: 'test-branch',
-        revision: '42b8736',
-        displayColor: _42b8735,
-        group: '[test-branch]'
-      },
-      {
-        id: 'exp-83425',
-        revision: '1ba7bcd',
-        displayColor: _1ba7bcd,
-        group: '[exp-83425]'
-      }
-    ],
     sectionName: DEFAULT_SECTION_NAMES[Section.COMPARISON_TABLE],
     size: PlotSize.REGULAR
   }

diff --git a/extension/src/test/fixtures/plotsDiff/revisions.ts b/extension/src/test/fixtures/plotsDiff/revisions.ts
@@ -0,0 +1,5 @@
+import { getRevisions } from '.'
+
+const data = getRevisions()
+
+export default data
diff --git a/extension/src/test/suite/experiments/model/tree.test.ts b/extension/src/test/suite/experiments/model/tree.test.ts
@@ -284,9 +284,7 @@ suite('Experiments Tree Test Suite', () => {
         messageSpy,
         'the same experiments are still selected'
       ).to.be.calledWithMatch({
-        comparison: {
-          revisions: initiallySelectedRevisions
-        }
+        selectedRevisions: initiallySelectedRevisions
       })
       setSelectionModeSpy.resetHistory()
       messageSpy.resetHistory()

diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts
@@ -12,6 +12,7 @@ import checkpointPlotsFixture from '../../fixtures/expShow/checkpointPlots'
 import plotsDiffFixture from '../../fixtures/plotsDiff/output'
 import templatePlotsFixture from '../../fixtures/plotsDiff/template'
 import comparisonPlotsFixture from '../../fixtures/plotsDiff/comparison/vscode'
+import plotsRevisionsFixture from '../../fixtures/plotsDiff/revisions'
 import {
   bypassProcessManagerDebounce,
   closeAllEditors,
@@ -449,14 +450,12 @@ suite('Plots Test Suite', () => {
         messageSpy,
         "should update the webview's comparison revision state"
       ).to.be.calledWithExactly({
-        comparison: {
-          ...comparisonPlotsFixture,
-          revisions: reorderObjectList(
-            mockComparisonOrder,
-            comparisonPlotsFixture.revisions,
-            'revision'
-          )
-        }
+        comparison: comparisonPlotsFixture,
+        selectedRevisions: reorderObjectList(
+          mockComparisonOrder,
+          plotsRevisionsFixture,
+          'revision'
+        )
       })
       expect(mockSendTelemetryEvent).to.be.calledOnce
       expect(mockSendTelemetryEvent).to.be.calledWithExactly(
@@ -723,8 +722,8 @@ suite('Plots Test Suite', () => {
         comparison: comparisonPlotsFixture,
         hasPlots: true,
         hasSelectedPlots: true,
-        hasSelectedRevisions: true,
         sectionCollapsed: DEFAULT_SECTION_COLLAPSED,
+        selectedRevisions: plotsRevisionsFixture,
         template: templatePlotsFixture
       }
 

diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx
@@ -13,6 +13,7 @@ import {
 } from '@testing-library/react'
 import '@testing-library/jest-dom/extend-expect'
 import comparisonTableFixture from 'dvc/src/test/fixtures/plotsDiff/comparison'
+import plotsRevisionsFixture from 'dvc/src/test/fixtures/plotsDiff/revisions'
 import checkpointPlotsFixture, {
   manyCheckpointPlots
 } from 'dvc/src/test/fixtures/expShow/checkpointPlots'
@@ -172,8 +173,8 @@ describe('App', () => {
       checkpoint: null,
       hasPlots: true,
       hasSelectedPlots: false,
-      hasSelectedRevisions: false,
-      sectionCollapsed: DEFAULT_SECTION_COLLAPSED
+      sectionCollapsed: DEFAULT_SECTION_COLLAPSED,
+      selectedRevisions: undefined
     })
     const addPlotsButton = await screen.findByText('Add Plots')
     const addExperimentsButton = await screen.findByText('Add Experiments')
@@ -890,6 +891,7 @@ describe('App', () => {
     renderAppWithData({
       comparison: comparisonTableFixture,
       sectionCollapsed: DEFAULT_SECTION_COLLAPSED,
+      selectedRevisions: plotsRevisionsFixture,
       template: complexTemplatePlotsFixture
     })
 
@@ -1007,7 +1009,8 @@ describe('App', () => {
   it('should not open a modal with the plot zoomed in when clicking a comparison table plot', () => {
     renderAppWithData({
       comparison: comparisonTableFixture,
-      sectionCollapsed: DEFAULT_SECTION_COLLAPSED
+      sectionCollapsed: DEFAULT_SECTION_COLLAPSED,
+      selectedRevisions: plotsRevisionsFixture
     })
 
     expect(screen.queryByTestId('modal')).not.toBeInTheDocument()
@@ -1445,7 +1448,8 @@ describe('App', () => {
     it('should show the revisions at the top', () => {
       renderAppWithData({
         comparison: comparisonTableFixture,
-        sectionCollapsed: DEFAULT_SECTION_COLLAPSED
+        sectionCollapsed: DEFAULT_SECTION_COLLAPSED,
+        selectedRevisions: plotsRevisionsFixture
       })
       const ribbon = screen.getByTestId('ribbon')
 
@@ -1454,7 +1458,7 @@ describe('App', () => {
       revisionBlocks.shift() // Remove refresh all
       const revisions = revisionBlocks.map(item => item.textContent)
       expect(revisions).toStrictEqual(
-        comparisonTableFixture.revisions.map(rev =>
+        plotsRevisionsFixture.map(rev =>
           rev.group ? rev.group.slice(1, -1) + rev.revision : rev.revision
         )
       )
@@ -1463,7 +1467,8 @@ describe('App', () => {
     it('should send a message with the revision to be removed when clicking the clear button', () => {
       renderAppWithData({
         comparison: comparisonTableFixture,
-        sectionCollapsed: DEFAULT_SECTION_COLLAPSED
+        sectionCollapsed: DEFAULT_SECTION_COLLAPSED,
+        selectedRevisions: plotsRevisionsFixture
       })
 
       const mainClearButton = within(
@@ -1481,11 +1486,12 @@ describe('App', () => {
     it('should display the number of experiments selected', () => {
       renderAppWithData({
         comparison: comparisonTableFixture,
-        sectionCollapsed: DEFAULT_SECTION_COLLAPSED
+        sectionCollapsed: DEFAULT_SECTION_COLLAPSED,
+        selectedRevisions: plotsRevisionsFixture
       })
 
       expect(
-        screen.getByText(`${comparisonTableFixture.revisions.length} of 7`)
+        screen.getByText(`${plotsRevisionsFixture.length} of 7`)
       ).toBeInTheDocument()
     })
 
@@ -1509,7 +1515,8 @@ describe('App', () => {
     it('should send a message to refresh each revision when clicking the refresh all button', () => {
       renderAppWithData({
         comparison: comparisonTableFixture,
-        sectionCollapsed: DEFAULT_SECTION_COLLAPSED
+        sectionCollapsed: DEFAULT_SECTION_COLLAPSED,
+        selectedRevisions: plotsRevisionsFixture
       })
 
       const refreshAllButton = within(

diff --git a/webview/src/plots/components/Plots.tsx b/webview/src/plots/components/Plots.tsx
@@ -63,7 +63,7 @@ const PlotsContent = ({ state }: PlotsProps) => {
     comparison: comparisonTable,
     hasPlots,
     hasSelectedPlots,
-    hasSelectedRevisions,
+    selectedRevisions,
     sectionCollapsed,
     template: templatePlots
   } = data
@@ -74,7 +74,7 @@ const PlotsContent = ({ state }: PlotsProps) => {
         addItems={
           <AddPlots
             hasSelectedPlots={!!hasSelectedPlots}
-            hasSelectedRevisions={!!hasSelectedRevisions}
+            hasSelectedRevisions={!!selectedRevisions?.length}
           />
         }
         showEmpty={!hasPlots}
@@ -119,7 +119,7 @@ const PlotsContent = ({ state }: PlotsProps) => {
 
   return (
     <>
-      <Ribbon revisions={comparisonTable?.revisions || []} />
+      <Ribbon revisions={selectedRevisions || []} />
       <DragDropProvider>
         <PlotsSizeProvider
           sizes={{
@@ -137,6 +137,7 @@ const PlotsContent = ({ state }: PlotsProps) => {
           {comparisonTable && (
             <ComparisonTableWrapper
               comparisonTable={comparisonTable}
+              revisions={selectedRevisions || []}
               {...wrapperProps}
             />
           )}

diff --git a/webview/src/plots/components/comparisonTable/ComparisonTable.test.tsx b/webview/src/plots/components/comparisonTable/ComparisonTable.test.tsx
@@ -11,6 +11,7 @@ import {
 } from '@testing-library/react'
 import { MessageFromWebviewType } from 'dvc/src/webview/contract'
 import comparisonTableFixture from 'dvc/src/test/fixtures/plotsDiff/comparison'
+import plotsRevisionsFixture from 'dvc/src/test/fixtures/plotsDiff/revisions'
 import React from 'react'
 import { Revision } from 'dvc/src/plots/webview/contract'
 import { ComparisonTable, ComparisonTableProps } from './ComparisonTable'
@@ -38,7 +39,10 @@ describe('ComparisonTable', () => {
     jest.clearAllMocks()
   })
 
-  const basicProps: ComparisonTableProps = comparisonTableFixture
+  const basicProps: ComparisonTableProps = {
+    ...comparisonTableFixture,
+    revisions: plotsRevisionsFixture
+  }
   const revisions = basicProps.revisions.map(({ revision }) => revision)
   const namedRevisions = basicProps.revisions.map(
     ({ revision, group }) => `${revision}${group || ''}`

diff --git a/webview/src/plots/components/comparisonTable/ComparisonTable.tsx b/webview/src/plots/components/comparisonTable/ComparisonTable.tsx
@@ -16,7 +16,7 @@ import { withScale } from '../../../util/styles'
 import { sendMessage } from '../../../shared/vscode'
 
 export type ComparisonTableProps = Omit<
-  PlotsComparisonData,
+  PlotsComparisonData & { revisions: Revision[] },
   'sectionName' | 'size'
 >
 

diff --git a/webview/src/plots/components/comparisonTable/ComparisonTableWrapper.tsx b/webview/src/plots/components/comparisonTable/ComparisonTableWrapper.tsx
@@ -1,18 +1,24 @@
-import { PlotsComparisonData, Section } from 'dvc/src/plots/webview/contract'
+import {
+  PlotsComparisonData,
+  Revision,
+  Section
+} from 'dvc/src/plots/webview/contract'
 import React from 'react'
 import { ComparisonTable } from './ComparisonTable'
 import { BasicContainerProps, PlotsContainer } from '../PlotsContainer'
 
 interface ComparisonTableWrapper {
   comparisonTable: PlotsComparisonData
+  revisions: Revision[]
   basicContainerProps: BasicContainerProps
 }
 
 export const ComparisonTableWrapper: React.FC<ComparisonTableWrapper> = ({
   comparisonTable,
+  revisions,
   basicContainerProps
 }) => {
-  const { revisions, sectionName, size, plots } = comparisonTable
+  const { sectionName, size, plots } = comparisonTable
 
   return (
     <PlotsContainer

diff --git a/webview/src/stories/ComparisonTable.stories.tsx b/webview/src/stories/ComparisonTable.stories.tsx
@@ -3,6 +3,7 @@ import { fireEvent, within } from '@testing-library/react'
 import React from 'react'
 import { ComparisonRevisionData } from 'dvc/src/plots/webview/contract'
 import comparisonTableFixture from 'dvc/src/test/fixtures/plotsDiff/comparison'
+import plotsRevisionsFixture from 'dvc/src/test/fixtures/plotsDiff/revisions'
 import {
   ComparisonTable,
   ComparisonTableProps
@@ -11,7 +12,7 @@ import { WebviewWrapper } from '../shared/components/webviewWrapper/WebviewWrapp
 import { DragDropProvider } from '../shared/components/dragDrop/DragDropContext'
 
 export default {
-  args: comparisonTableFixture,
+  args: { ...comparisonTableFixture, revisions: plotsRevisionsFixture },
   component: ComparisonTable,
   title: 'Comparison Table'
 } as Meta