UnresolvedFile Action RenderBug

apps/demo/src/main.ts

@@ -48,7 +48,7 @@ const DEMO_THEME: DiffsThemeNames | ThemesType = DEFAULT_THEMES;
 const WORKER_POOL = true;
 const VIRTUALIZE = true;
 const CRAZY_FILE = false;
-const LARGE_CONFLICT_FILE = false;
+const LARGE_CONFLICT_FILE = true;
 
 const FileStreamCodeConfigs: FileStreamCodeConfigsItem[] = [
   {

packages/diffs/src/components/File.ts

@@ -49,7 +49,7 @@ import { upsertHostThemeStyle } from '../utils/hostTheme';
 import { prerenderHTMLIfNecessary } from '../utils/prerenderHTMLIfNecessary';
 import { getMeasuredScrollbarGutter } from '../utils/scrollbarGutter';
 import { setPreNodeProperties } from '../utils/setWrapperNodeProps';
-import type { WorkerPoolManager } from '../worker';
+import type { HighlightRequestMetadata, WorkerPoolManager } from '../worker';
 import { DiffsContainerLoaded } from './web-components';
 
 const EMPTY_STRINGS: string[] = [];
@@ -166,7 +166,7 @@ export class File<LAnnotation = undefined> {
   ) {
     this.fileRenderer = new FileRenderer<LAnnotation>(
       options,
-      this.handleHighlightRender,
+      (metadata) => this.handleHighlightRender(metadata),
       this.workerManager
     );
     this.resizeManager = new ResizeManager();
@@ -177,9 +177,9 @@ export class File<LAnnotation = undefined> {
     this.workerManager?.subscribeToThemeChanges(this);
   }
 
-  private handleHighlightRender = (): void => {
+  private handleHighlightRender(_metadata?: HighlightRequestMetadata): void {
     this.rerender();
-  };
+  }
 
   public rerender(): void {
     if (this.file == null) return;

packages/diffs/src/components/FileDiff.ts

@@ -64,7 +64,7 @@ import { parseDiffFromFile } from '../utils/parseDiffFromFile';
 import { prerenderHTMLIfNecessary } from '../utils/prerenderHTMLIfNecessary';
 import { getMeasuredScrollbarGutter } from '../utils/scrollbarGutter';
 import { setPreNodeProperties } from '../utils/setWrapperNodeProps';
-import type { WorkerPoolManager } from '../worker';
+import type { HighlightRequestMetadata, WorkerPoolManager } from '../worker';
 import { DiffsContainerLoaded } from './web-components';
 
 export interface FileDiffRenderProps<LAnnotation> {
@@ -243,9 +243,9 @@ export class FileDiff<LAnnotation = undefined> {
     this.enabled = true;
   }
 
-  protected handleHighlightRender = (): void => {
+  protected handleHighlightRender(_metadata?: HighlightRequestMetadata): void {
     this.rerender();
-  };
+  }
 
   protected getHunksRendererOptions(
     options: FileDiffOptions<LAnnotation>
@@ -266,7 +266,7 @@ export class FileDiff<LAnnotation = undefined> {
   ): DiffHunksRenderer<LAnnotation> {
     return new DiffHunksRenderer(
       this.getHunksRendererOptions(options),
-      this.handleHighlightRender,
+      (metadata) => this.handleHighlightRender(metadata),
       this.workerManager
     );
   }

packages/diffs/src/components/UnresolvedFile.ts

@@ -13,6 +13,7 @@ import type {
   MergeConflictMarkerRow,
   MergeConflictRegion,
   MergeConflictResolution,
+  RenderRange,
 } from '../types';
 import { areFilesEqual } from '../utils/areFilesEqual';
 import { areMergeConflictActionsEqual } from '../utils/areMergeConflictActionsEqual';
@@ -26,7 +27,7 @@ import {
 } from '../utils/parseMergeConflictDiffFromFile';
 import { resolveConflict as resolveConflictDiff } from '../utils/resolveConflict';
 import { splitFileContents } from '../utils/splitFileContents';
-import type { WorkerPoolManager } from '../worker';
+import type { HighlightRequestMetadata, WorkerPoolManager } from '../worker';
 import {
   FileDiff,
   type FileDiffOptions,
@@ -106,6 +107,24 @@ interface ResolveConflictReturn {
   markerRows: MergeConflictMarkerRow[];
 }
 
+interface PendingRender<LAnnotation> {
+  fileDiff: FileDiffMetadata;
+  actions: (MergeConflictDiffAction | undefined)[];
+  markerRows: MergeConflictMarkerRow[];
+  lineAnnotations: UnresolvedFileRenderProps<LAnnotation>['lineAnnotations'];
+  renderRange: RenderRange | undefined;
+  preventEmit: boolean;
+  forceRender?: boolean;
+  fileContainer?: HTMLElement;
+  containerWrapper?: HTMLElement;
+}
+
+interface ActiveMergeConflictState {
+  actions: (MergeConflictDiffAction | undefined)[];
+  markerRows: MergeConflictMarkerRow[];
+  fileDiff: FileDiffMetadata | undefined;
+}
+
 type UnresolvedFileDataCache = GetOrComputeDiffProps;
 
 let instanceId = -1;
@@ -124,6 +143,7 @@ export class UnresolvedFile<
   private markerRows: MergeConflictMarkerRow[] = [];
   private conflictActionCache: Map<string, MergeConflictActionElementCache> =
     new Map();
+  private pendingRender: PendingRender<LAnnotation> | undefined;
 
   constructor(
     public override options: UnresolvedFileOptions<LAnnotation> = {
@@ -175,7 +195,7 @@ export class UnresolvedFile<
   ): UnresolvedFileHunksRenderer<LAnnotation> {
     const renderer = new UnresolvedFileHunksRenderer<LAnnotation>(
       this.getHunksRendererOptions(options),
-      this.handleHighlightRender,
+      (metadata) => this.handleHighlightRender(metadata),
       this.workerManager
     );
     return renderer;
@@ -205,6 +225,7 @@ export class UnresolvedFile<
       markerRows: undefined,
     };
     this.conflictActions = [];
+    this.pendingRender = undefined;
     super.cleanUp();
   }
 
@@ -354,7 +375,7 @@ export class UnresolvedFile<
       return;
     }
     this.hydrateElements(fileContainer, prerenderedHTML);
-    this.setActiveMergeConflictState(source.actions, source.markerRows);
+    this.setActiveMergeConflictState(source);
     // If necessary hydration elements don't exist, we should assume a full
     // render
     if (
@@ -386,6 +407,27 @@ export class UnresolvedFile<
     this.render({ forceRender: true, renderRange: this.renderRange });
   }
 
+  protected override handleHighlightRender(
+    metadata?: HighlightRequestMetadata
+  ): void {
+    console.log('handleHighlightRender', metadata);
+    const { pendingRender } = this;
+    if (metadata != null) {
+      if (metadata === pendingRender) {
+        this.pendingRender = undefined;
+        console.log('handleHighlightRender.rendering a pre render');
+        this.renderResolvedState(pendingRender, true);
+      }
+      console.log('handleHighlightRender.throwing away a render');
+      // If we get in here, then it means we are waiting on a render that's
+      // been invalidated, so drop it
+      return;
+    }
+
+    console.log('handleHighlightRender.rendering normie render');
+    super.handleHighlightRender(metadata);
+  }
+
   override render(props: UnresolvedFileRenderProps<LAnnotation> = {}): boolean {
     let {
       file,
@@ -405,20 +447,30 @@ export class UnresolvedFile<
     if (source == null) {
       return false;
     }
-    this.setActiveMergeConflictState(source.actions, source.markerRows);
-    const didRender = super.render({
-      ...rest,
+    const nextRender: PendingRender<LAnnotation> = {
       fileDiff: source.fileDiff,
+      actions: source.actions,
+      markerRows: source.markerRows,
       lineAnnotations,
-      preventEmit: true,
-    });
-    if (didRender) {
-      this.renderMergeConflictActionSlots();
-      if (!preventEmit) {
-        this.emitPostRender();
-      }
+      renderRange: rest.renderRange,
+      preventEmit,
+      forceRender: rest.forceRender,
+      fileContainer: rest.fileContainer,
+      containerWrapper: rest.containerWrapper,
+    };
+
+    if (this.shouldDeferRender(source.fileDiff)) {
+      console.log('queing render');
+      this.queuePendingRender(nextRender);
+      return false;
     }
-    return didRender;
+
+    // if (this.pendingRender?.fileDiff === source.fileDiff) {
+    // }
+
+    console.log('linear render');
+    this.pendingRender = undefined;
+    return this.renderResolvedState(nextRender);
   }
 
   public resolveConflict(
@@ -486,37 +538,93 @@ export class UnresolvedFile<
     }
 
     this.computedCache = { file, fileDiff, actions, markerRows };
-    this.setActiveMergeConflictState(actions, markerRows);
-    // NOTE(amadeus): This is a bit jank, but helps to ensure we don't see a
-    // bunch of jittery re-renders as things resolve out.  In a more perfect
-    // world we would have a more elegant way to kick off a render to the
-    // highlighter and then resolve actions in a cleaner way, but time is short
-    // right now.  Can't let perfect be the enemy of good
-    if (this.workerManager != null) {
-      // Because we are using a workerManager, if we fire off the renderDiff
-      // call, it will eventually get back to us in a callback which will
-      // trigger a re-render
-      this.hunksRenderer.renderDiff(fileDiff);
+    const nextRender: PendingRender<LAnnotation> = {
+      fileDiff,
+      actions,
+      markerRows,
+      lineAnnotations: this.lineAnnotations,
+      renderRange: this.renderRange,
+      preventEmit: false,
+      forceRender: true,
+    };
+    if (this.shouldDeferRender(fileDiff)) {
+      console.log('resolveConflictAndRender: queued');
+      this.queuePendingRender(nextRender);
     } else {
-      this.render({ forceRender: true });
+      console.log('resolveConflictAndRender: linear');
+      this.pendingRender = undefined;
+      this.renderResolvedState(nextRender);
     }
     this.options.onMergeConflictResolve?.(file, payload);
   }
 
-  private setActiveMergeConflictState(
-    actions: (MergeConflictDiffAction | undefined)[] = this.conflictActions,
-    markerRows: MergeConflictMarkerRow[] = this.markerRows
-  ): void {
+  private shouldDeferRender(fileDiff: FileDiffMetadata): boolean {
+    return (
+      fileDiff !== this.fileDiff &&
+      this.hunksRenderer.willTriggerAsyncHighlight(fileDiff)
+    );
+  }
+
+  // The pending render payload is also the metadata passed through async
+  // highlighting so the completion callback can promote only the latest render.
+  private queuePendingRender(pendingRender: PendingRender<LAnnotation>): void {
+    this.pendingRender = pendingRender;
+    this.hunksRenderer.renderDiff(
+      pendingRender.fileDiff,
+      pendingRender.renderRange,
+      pendingRender
+    );
+  }
+
+  private renderResolvedState(
+    pendingRender: PendingRender<LAnnotation>,
+    forceRenderOverride = false
+  ): boolean {
+    const {
+      fileDiff,
+      actions,
+      markerRows,
+      lineAnnotations,
+      renderRange,
+      preventEmit,
+      forceRender = false,
+      fileContainer,
+      containerWrapper,
+    } = pendingRender;
+    this.setActiveMergeConflictState({ actions, markerRows, fileDiff });
+    const didRender = super.render({
+      fileDiff,
+      lineAnnotations,
+      renderRange,
+      forceRender: forceRenderOverride || forceRender,
+      fileContainer,
+      containerWrapper,
+      preventEmit: true,
+    });
+    if (didRender) {
+      this.renderMergeConflictActionSlots();
+      if (!preventEmit) {
+        this.emitPostRender();
+      }
+    }
+    return didRender;
+  }
+
+  private setActiveMergeConflictState({
+    actions = this.conflictActions,
+    markerRows = this.markerRows,
+    fileDiff = this.fileDiff,
+  }: ActiveMergeConflictState): void {
     this.conflictActions = actions;
     this.markerRows = markerRows;
     if (
-      this.computedCache.fileDiff != null &&
+      fileDiff != null &&
       this.hunksRenderer instanceof UnresolvedFileHunksRenderer
     ) {
       this.hunksRenderer.setConflictState(
         this.options.mergeConflictActionsType === 'none' ? [] : actions,
         markerRows,
-        this.computedCache.fileDiff
+        fileDiff
       );
     }
   }
@@ -534,6 +642,12 @@ export class UnresolvedFile<
         "UnresolvedFile.handleMergeConflictActionClick: conflictIndex and conflictAction don't match"
       );
     }
+    // NOTE(amadeus): Not sure if this will bite us or not... maybe we could
+    // take the active pending render data for this and still allow things to
+    // get triggered?  I'll need to test this ont he demo
+    if (this.pendingRender != null) {
+      return;
+    }
     const payload: MergeConflictActionPayload = {
       resolution: target.resolution,
       conflict: action.conflict,
@@ -546,7 +660,7 @@ export class UnresolvedFile<
   };
 
   private renderMergeConflictActionSlots(): void {
-    const { fileDiff } = this.computedCache;
+    const { fileDiff } = this;
     if (
       this.isContainerManaged ||
       this.fileContainer == null ||