Move git call into experiments data (#4036)

* move number of commits call into experiments data

* refactor and tidy

Author: mattseddon

extension/src/data/index.ts

@@ -10,9 +10,10 @@ import { DeferredDisposable } from '../class/deferred'
 import { isSameOrChild } from '../fileSystem'
 
 export type ExperimentsOutput = {
+  availableNbCommits: { [branch: string]: number }
   expShow: ExpShowOutput
-  rowOrder: { branch: string; sha: string }[]
   gitLog: string
+  rowOrder: { branch: string; sha: string }[]
 }
 
 export abstract class BaseData<

extension/src/experiments/data/index.ts

@@ -51,10 +51,17 @@ export class ExperimentsData extends BaseData<ExperimentsOutput> {
     const branches = await this.getBranchesToShow(allBranches)
     let gitLog = ''
     const rowOrder: { branch: string; sha: string }[] = []
+    const availableNbCommits: { [branch: string]: number } = {}
     const args: Args = []
 
     for (const branch of branches) {
-      gitLog = await this.collectGitLogAndOrder(gitLog, branch, rowOrder, args)
+      gitLog = await this.collectGitLogAndOrder(
+        gitLog,
+        branch,
+        rowOrder,
+        availableNbCommits,
+        args
+      )
     }
 
     const expShow = await this.internalCommands.executeCommand<ExpShowOutput>(
@@ -65,11 +72,7 @@ export class ExperimentsData extends BaseData<ExperimentsOutput> {
 
     this.collectFiles({ expShow })
 
-    return this.notifyChanged({
-      expShow,
-      gitLog,
-      rowOrder
-    })
+    return this.notifyChanged({ availableNbCommits, expShow, gitLog, rowOrder })
   }
 
   protected collectFiles({ expShow }: { expShow: ExpShowOutput }) {
@@ -80,17 +83,26 @@ export class ExperimentsData extends BaseData<ExperimentsOutput> {
     gitLog: string,
     branch: string,
     rowOrder: { branch: string; sha: string }[],
+    availableNbCommits: { [branch: string]: number },
     args: Args
   ) {
     const nbOfCommitsToShow = this.experiments.getNbOfCommitsToShow(branch)
 
-    const branchGitLog = await this.internalCommands.executeCommand(
-      AvailableCommands.GIT_GET_COMMIT_MESSAGES,
-      this.dvcRoot,
-      branch,
-      String(nbOfCommitsToShow)
-    )
+    const [branchGitLog, totalCommits] = await Promise.all([
+      this.internalCommands.executeCommand(
+        AvailableCommands.GIT_GET_COMMIT_MESSAGES,
+        this.dvcRoot,
+        branch,
+        String(nbOfCommitsToShow)
+      ),
+      this.internalCommands.executeCommand<number>(
+        AvailableCommands.GIT_GET_NUM_COMMITS,
+        this.dvcRoot,
+        branch
+      )
+    ])
     gitLog = [gitLog, branchGitLog].join(COMMITS_SEPARATOR)
+    availableNbCommits[branch] = totalCommits
 
     for (const commit of branchGitLog.split(COMMITS_SEPARATOR)) {
       const [sha] = commit.split('\n')

extension/src/experiments/index.ts

@@ -168,12 +168,23 @@ export class Experiments extends BaseRepository<TableData> {
     return this.data.managedUpdate()
   }
 
-  public async setState({ expShow, gitLog, rowOrder }: ExperimentsOutput) {
+  public async setState({
+    availableNbCommits,
+    expShow,
+    gitLog,
+    rowOrder
+  }: ExperimentsOutput) {
     const hadCheckpoints = this.hasCheckpoints()
     const dvcLiveOnly = await this.checkSignalFile()
     await Promise.all([
       this.columns.transformAndSet(expShow),
-      this.experiments.transformAndSet(expShow, gitLog, dvcLiveOnly, rowOrder)
+      this.experiments.transformAndSet(
+        expShow,
+        gitLog,
+        dvcLiveOnly,
+        rowOrder,
+        availableNbCommits
+      )
     ])
 
     if (hadCheckpoints !== this.hasCheckpoints()) {
@@ -551,12 +562,6 @@ export class Experiments extends BaseRepository<TableData> {
         ),
       () => this.addStage(),
       (branchesSelected: string[]) => this.selectBranches(branchesSelected),
-      (branch: string) =>
-        this.internalCommands.executeCommand<number>(
-          AvailableCommands.GIT_GET_NUM_COMMITS,
-          this.dvcRoot,
-          branch
-        ),
       () => this.data.update()
     )
 

extension/src/experiments/model/collect.ts

@@ -186,6 +186,28 @@ const collectExpState = (
   return baseline
 }
 
+export const collectAddRemoveCommitsDetails = (
+  availableNbCommits: {
+    [branch: string]: number
+  },
+  getNbOfCommitsToShow: (branch: string) => number
+): {
+  hasMoreCommits: { [branch: string]: boolean }
+  isShowingMoreCommits: { [branch: string]: boolean }
+} => {
+  const hasMoreCommits: { [branch: string]: boolean } = {}
+  const isShowingMoreCommits: { [branch: string]: boolean } = {}
+
+  for (const [branch, availableCommits] of Object.entries(availableNbCommits)) {
+    const nbOfCommitsToShow = getNbOfCommitsToShow(branch)
+    hasMoreCommits[branch] = availableCommits > nbOfCommitsToShow
+    isShowingMoreCommits[branch] =
+      Math.min(nbOfCommitsToShow, availableCommits) > 1
+  }
+
+  return { hasMoreCommits, isShowingMoreCommits }
+}
+
 const getExecutor = (experiment: Experiment): Executor => {
   if ([experiment.executor, experiment.id].includes(EXPERIMENT_WORKSPACE_ID)) {
     return Executor.WORKSPACE

extension/src/experiments/model/index.test.ts

@@ -33,26 +33,39 @@ beforeEach(() => {
   jest.resetAllMocks()
 })
 
-const DEFAULT_DATA: [string, boolean, { branch: string; sha: string }[]] = [
-  '',
-  false,
-  []
-]
+const DEFAULT_DATA: [
+  string,
+  boolean,
+  { branch: string; sha: string }[],
+  { [branch: string]: number }
+] = ['', false, [], { main: 2000 }]
 
 describe('ExperimentsModel', () => {
   it('should return the expected rows when given the base fixture', () => {
     const model = new ExperimentsModel('', buildMockMemento())
-    model.transformAndSet(outputFixture, gitLogFixture, false, rowOrderFixture)
+    model.transformAndSet(
+      outputFixture,
+      gitLogFixture,
+      false,
+      rowOrderFixture,
+      { main: 6 }
+    )
     expect(model.getRowData()).toStrictEqual(rowsFixture)
   })
 
   it('should return the expected rows when given the survival fixture', () => {
     const model = new ExperimentsModel('', buildMockMemento())
-    model.transformAndSet(survivalOutputFixture, '', false, [
-      { branch: 'main', sha: '3d5adcb974bb2c85917a5d61a489b933adaa2b7f' },
-      { branch: 'main', sha: 'a49e03966a1f9f1299ec222ebc4bed8625d2c54d' },
-      { branch: 'main', sha: '4f7b50c3d171a11b6cfcd04416a16fc80b61018d' }
-    ])
+    model.transformAndSet(
+      survivalOutputFixture,
+      '',
+      false,
+      [
+        { branch: 'main', sha: '3d5adcb974bb2c85917a5d61a489b933adaa2b7f' },
+        { branch: 'main', sha: 'a49e03966a1f9f1299ec222ebc4bed8625d2c54d' },
+        { branch: 'main', sha: '4f7b50c3d171a11b6cfcd04416a16fc80b61018d' }
+      ],
+      { main: 700 }
+    )
     expect(model.getRowData()).toStrictEqual(
       expect.objectContaining(survivalRowsFixture)
     )
@@ -87,7 +100,7 @@ describe('ExperimentsModel', () => {
       }
     )
 
-    model.transformAndSet(dvcLiveOnly, '', true, [])
+    model.transformAndSet(dvcLiveOnly, '', true, [], { main: 2000 })
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     const runningWorkspace = (model as any).workspace
     expect(runningWorkspace?.executor).toStrictEqual(EXPERIMENT_WORKSPACE_ID)
@@ -178,19 +191,27 @@ describe('ExperimentsModel', () => {
 
   it('should return the expected rows when given the deeply nested output fixture', () => {
     const model = new ExperimentsModel('', buildMockMemento())
-    model.transformAndSet(deeplyNestedOutputFixture, '', false, [
-      { branch: 'main', sha: '53c3851f46955fa3e2b8f6e1c52999acc8c9ea77' }
-    ])
+    model.transformAndSet(
+      deeplyNestedOutputFixture,
+      '',
+      false,
+      [{ branch: 'main', sha: '53c3851f46955fa3e2b8f6e1c52999acc8c9ea77' }],
+      { main: 10 }
+    )
     expect(model.getRowData()).toStrictEqual(
       expect.objectContaining(deeplyNestedRowsFixture)
     )
   })
 
   it('should return the expected rows when given the data types output fixture', () => {
     const model = new ExperimentsModel('', buildMockMemento())
-    model.transformAndSet(dataTypesOutputFixture, '', false, [
-      { branch: 'main', sha: '53c3851f46955fa3e2b8f6e1c52999acc8c9ea77' }
-    ])
+    model.transformAndSet(
+      dataTypesOutputFixture,
+      '',
+      false,
+      [{ branch: 'main', sha: '53c3851f46955fa3e2b8f6e1c52999acc8c9ea77' }],
+      { main: 10 }
+    )
     expect(model.getRowData()).toStrictEqual(
       expect.objectContaining(dataTypesRowsFixture)
     )

extension/src/experiments/model/index.ts

@@ -2,6 +2,7 @@ import { Memento } from 'vscode'
 import { SortDefinition, sortExperiments } from './sortBy'
 import { FilterDefinition, filterExperiment, getFilterId } from './filterBy'
 import {
+  collectAddRemoveCommitsDetails,
   collectExperiments,
   collectOrderedCommitsAndExperiments,
   collectRunningInQueue,
@@ -62,6 +63,8 @@ export class ExperimentsModel extends ModelWithPersistence {
   private numberOfCommitsToShow: Record<string, number>
   private branchesToShow: string[] = []
   private availableBranchesToShow: string[] = []
+  private hasMoreCommits: { [branch: string]: boolean } = {}
+  private isShowingMoreCommits: { [branch: string]: boolean } = {}
 
   private filters: Map<string, FilterDefinition> = new Map()
 
@@ -115,7 +118,8 @@ export class ExperimentsModel extends ModelWithPersistence {
     expShow: ExpShowOutput,
     gitLog: string,
     dvcLiveOnly: boolean,
-    rowOrder: { branch: string; sha: string }[]
+    rowOrder: { branch: string; sha: string }[],
+    availableNbCommits: { [branch: string]: number }
   ) {
     const {
       workspace,
@@ -125,6 +129,14 @@ export class ExperimentsModel extends ModelWithPersistence {
       hasCheckpoints
     } = collectExperiments(expShow, gitLog, dvcLiveOnly)
 
+    const { hasMoreCommits, isShowingMoreCommits } =
+      collectAddRemoveCommitsDetails(availableNbCommits, (branch: string) =>
+        this.getNbOfCommitsToShow(branch)
+      )
+
+    this.hasMoreCommits = hasMoreCommits
+    this.isShowingMoreCommits = isShowingMoreCommits
+
     commits.sort((a, b) => (b.Created || '').localeCompare(a.Created || ''))
 
     this.rowOrder = rowOrder
@@ -383,6 +395,14 @@ export class ExperimentsModel extends ModelWithPersistence {
     ]
   }
 
+  public getHasMoreCommits() {
+    return this.hasMoreCommits
+  }
+
+  public getIsShowingMoreCommits() {
+    return this.isShowingMoreCommits
+  }
+
   public isSelected(id: string) {
     return !!this.coloredStatus[id]
   }

extension/src/experiments/webview/messages.ts

@@ -37,15 +37,12 @@ export class WebviewMessages {
 
   private hasConfig = false
   private hasValidDvcYaml = true
-  private hasMoreCommits: Record<string, boolean> = {}
-  private isShowingMoreCommits: Record<string, boolean> = {}
 
   private readonly addStage: () => Promise<boolean>
   private readonly selectBranches: (
     branchesSelected: string[]
   ) => Promise<string[] | undefined>
 
-  private readonly getNumCommits: (branch: string) => Promise<number>
   private readonly update: () => Promise<void>
 
   constructor(
@@ -60,7 +57,6 @@ export class WebviewMessages {
     selectBranches: (
       branchesSelected: string[]
     ) => Promise<string[] | undefined>,
-    getNumCommits: (branch: string) => Promise<number>,
     update: () => Promise<void>
   ) {
     this.dvcRoot = dvcRoot
@@ -72,11 +68,9 @@ export class WebviewMessages {
     this.hasStages = hasStages
     this.addStage = addStage
     this.selectBranches = selectBranches
-    this.getNumCommits = getNumCommits
     this.update = update
 
     void this.changeHasConfig()
-    void this.changeHasMoreOrLessCommits()
   }
 
   public async changeHasConfig(update?: boolean) {
@@ -86,10 +80,7 @@ export class WebviewMessages {
     update && this.sendWebviewMessage()
   }
 
-  public sendWebviewMessage(doNotCheckNbCommits?: boolean) {
-    if (!doNotCheckNbCommits) {
-      void this.changeHasMoreOrLessCommits(true)
-    }
+  public sendWebviewMessage() {
     const webview = this.getWebview()
     void webview?.show(this.getWebviewData())
   }
@@ -233,19 +224,6 @@ export class WebviewMessages {
     )
     this.experiments.setBranchesToShow(selectedBranches)
     await this.update()
-    await this.changeHasMoreOrLessCommits(true)
-  }
-
-  private async changeHasMoreOrLessCommits(update?: boolean) {
-    for (const branch of this.experiments.getBranchesToShow()) {
-      const availableNbCommits = await this.getNumCommits(branch)
-      const nbOfCommitsToShow = this.experiments.getNbOfCommitsToShow(branch)
-      this.hasMoreCommits[branch] = availableNbCommits > nbOfCommitsToShow
-      this.isShowingMoreCommits[branch] =
-        Math.min(nbOfCommitsToShow, availableNbCommits) > 1
-    }
-
-    update && this.sendWebviewMessage(true)
   }
 
   private async changeCommitsToShow(change: 1 | -1, branch: string) {
@@ -262,7 +240,6 @@ export class WebviewMessages {
       branch
     )
     await this.update()
-    await this.changeHasMoreOrLessCommits(true)
   }
 
   private getWebviewData() {
@@ -278,11 +255,11 @@ export class WebviewMessages {
       hasCheckpoints: this.experiments.hasCheckpoints(),
       hasColumns: this.columns.hasNonDefaultColumns(),
       hasConfig: this.hasConfig,
-      hasMoreCommits: this.hasMoreCommits,
+      hasMoreCommits: this.experiments.getHasMoreCommits(),
       hasRunningWorkspaceExperiment:
         this.experiments.hasRunningWorkspaceExperiment(),
       hasValidDvcYaml: this.hasValidDvcYaml,
-      isShowingMoreCommits: this.isShowingMoreCommits,
+      isShowingMoreCommits: this.experiments.getIsShowingMoreCommits(),
       rows: this.experiments.getRowData(),
       selectedForPlotsCount: this.experiments.getSelectedRevisions().length,
       sorts: this.experiments.getSorts()