V48 Gate 3 (implementation-only): stop deposit file-tree pickers from flickering empty on treeRef thrash

geraldarthurdavis · geraldarthurdavis · commit a717025ef263 · 2026-07-08T13:51:58.000-07:00
Abort + generation-guard in-flight tree fetches so a late empty/error response from a prior branch package cannot wipe a successful commit-package load.
diff --git a/uapi/components/base/bitcode/vcs/VCSFileTreePicker.tsx b/uapi/components/base/bitcode/vcs/VCSFileTreePicker.tsx
@@ -11,7 +11,7 @@
  * given conflict label. Square theme, no card-in-card chrome.
  */
 
-import React, { useCallback, useEffect, useMemo, useState } from 'react';
+import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react';
 import { ChevronDown, ChevronRight, FileText, Folder } from 'lucide-react';
 import type { VCSProviderType } from '@bitcode/vcs-core';
 
@@ -45,6 +45,27 @@ function baseName(path: string) {
   return index === -1 ? trimmed : trimmed.slice(index + 1);
 }
 
+function normalizeTreeItems(raw: unknown): VCSFileTreePickerTreeItem[] {
+  if (!Array.isArray(raw)) return [];
+  return raw
+    .filter(
+      (item: { path?: unknown; type?: unknown }) =>
+        typeof item?.path === 'string' &&
+        (item?.type === 'blob' || item?.type === 'tree'),
+    )
+    .map((item: { path: string; type: 'blob' | 'tree' }) => ({
+      path: item.path,
+      type: item.type,
+    }))
+    .sort((a, b) =>
+      a.type === b.type
+        ? a.path.localeCompare(b.path)
+        : a.type === 'tree'
+          ? -1
+          : 1,
+    );
+}
+
 export function VCSFileTreePicker({
   provider,
   repositoryFullName,
@@ -63,6 +84,13 @@ export function VCSFileTreePicker({
   const [expanded, setExpanded] = useState<Record<string, boolean>>({});
   const [loadError, setLoadError] = useState<string | null>(null);
 
+  // Bumped on every tree-identity change (repo/ref) and on unmount so
+  // in-flight fetches from a prior package cannot write empty/error results
+  // over a later successful load (the flicker-to-empty race). Same pattern
+  // DepositSourceSelection uses with its disposed flags.
+  const loadGenerationRef = useRef(0);
+  const abortControllerRef = useRef<AbortController | null>(null);
+
   const [owner, repo] = useMemo(() => {
     const [ownerPart, repoPart] = (repositoryFullName || '').split('/');
     return [ownerPart || null, repoPart || null];
@@ -75,9 +103,15 @@ export function VCSFileTreePicker({
   const selectedSet = useMemo(() => new Set(selectedPaths), [selectedPaths]);
 
   const loadDirectory = useCallback(
-    async (path: string) => {
+    async (path: string, generation: number, signal: AbortSignal) => {
       if (!owner || !repo) return;
-      setChildrenByPath((previous) => ({ ...previous, [path]: previous[path] ?? null }));
+      // Only mark loading when we don't already have items for this path —
+      // keeps the tree painted during a soft re-fetch of an expanded dir.
+      setChildrenByPath((previous) =>
+        previous[path] === undefined
+          ? { ...previous, [path]: null }
+          : previous,
+      );
       try {
         const params = new URLSearchParams({
           resource: 'tree',
@@ -87,49 +121,93 @@ export function VCSFileTreePicker({
         });
         if (path) params.set('path', path);
         if (treeRef) params.set('ref', treeRef);
-        const response = await fetch(`/api/vcs?${params.toString()}`);
-        const payload = response?.ok ? await response.json().catch(() => null) : null;
-        const items: VCSFileTreePickerTreeItem[] = Array.isArray(payload?.items)
-          ? payload.items
-              .filter(
-                (item: { path?: unknown; type?: unknown }) =>
-                  typeof item?.path === 'string' &&
-                  (item?.type === 'blob' || item?.type === 'tree'),
-              )
-              .sort(
-                (a: VCSFileTreePickerTreeItem, b: VCSFileTreePickerTreeItem) =>
-                  a.type === b.type
-                    ? a.path.localeCompare(b.path)
-                    : a.type === 'tree'
-                      ? -1
-                      : 1,
-              )
-          : [];
+        const response = await fetch(`/api/vcs?${params.toString()}`, {
+          signal,
+        });
+        if (generation !== loadGenerationRef.current || signal.aborted) return;
+        const payload = response?.ok
+          ? await response.json().catch(() => null)
+          : null;
+        if (generation !== loadGenerationRef.current || signal.aborted) return;
+        if (!payload) {
+          // Failed read: keep any previously-good paint; only write empty
+          // when we never had items (null/undefined). Avoids the
+          // success→stale-failure→Empty directory flicker.
+          setChildrenByPath((previous) => {
+            const existing = previous[path];
+            if (Array.isArray(existing) && existing.length > 0) return previous;
+            return { ...previous, [path]: [] };
+          });
+          setLoadError('Unable to read the repository file tree.');
+          return;
+        }
+        const items = normalizeTreeItems(payload.items);
         setChildrenByPath((previous) => ({ ...previous, [path]: items }));
-        if (!payload) setLoadError('Unable to read the repository file tree.');
-      } catch {
-        setChildrenByPath((previous) => ({ ...previous, [path]: [] }));
+        setLoadError(null);
+      } catch (error) {
+        const aborted =
+          signal.aborted ||
+          (error instanceof Error && error.name === 'AbortError');
+        if (generation !== loadGenerationRef.current || aborted) {
+          return;
+        }
+        setChildrenByPath((previous) => {
+          const existing = previous[path];
+          if (Array.isArray(existing) && existing.length > 0) return previous;
+          return { ...previous, [path]: [] };
+        });
         setLoadError('Unable to read the repository file tree.');
       }
     },
     [owner, provider, repo, treeRef],
   );
 
-  // Reset + load the root whenever the source package changes.
+  // Reset + load the root whenever the source package (owner/repo/ref) changes.
+  // treeRef thrashing is expected: DepositSourceSelection publishes branch
+  // first, then head commit once commits load — without abort + generation
+  // guards the branch-fetch can land after the commit-fetch and wipe it.
   useEffect(() => {
-    setChildrenByPath({});
+    const generation = loadGenerationRef.current + 1;
+    loadGenerationRef.current = generation;
+    abortControllerRef.current?.abort();
+    const controller = new AbortController();
+    abortControllerRef.current = controller;
+
     setExpanded({});
     setLoadError(null);
-    if (!owner || !repo) return;
-    void loadDirectory('');
+    if (!owner || !repo) {
+      setChildrenByPath({});
+      return () => {
+        controller.abort();
+      };
+    }
+
+    // Root reloads to Loading (null), not Empty ([]). Keeping a prior tree
+    // painted during ref hops would flash the wrong revision, so clear —
+    // but never seed with [] (that is the "Empty directory" state).
+    setChildrenByPath({ '': null });
+    void loadDirectory('', generation, controller.signal);
+
+    return () => {
+      // Invalidate every in-flight response for this package.
+      if (loadGenerationRef.current === generation) {
+        loadGenerationRef.current = generation + 1;
+      }
+      controller.abort();
+    };
   }, [loadDirectory, owner, repo]);
 
   const toggleExpanded = (path: string) => {
     setExpanded((previous) => {
       const next = { ...previous, [path]: !previous[path] };
       return next;
     });
-    if (childrenByPath[path] === undefined) void loadDirectory(path);
+    if (childrenByPath[path] === undefined) {
+      const controller = abortControllerRef.current;
+      if (controller && !controller.signal.aborted) {
+        void loadDirectory(path, loadGenerationRef.current, controller.signal);
+      }
+    }
   };
 
   const toggleSelected = (selectionPath: string) => {
diff --git a/uapi/tests/vcsFileTreePicker.test.tsx b/uapi/tests/vcsFileTreePicker.test.tsx
@@ -177,4 +177,83 @@ describe('VCSFileTreePicker', () => {
     fireEvent.click(screen.getByRole('button', { name: 'Clear all' }));
     expect(onChange).toHaveBeenCalledWith([]);
   });
+
+  it('ignores a stale tree response when treeRef hops (no empty flicker)', async () => {
+    // Reproduce the deposit source-selection thrash: branch name is published
+    // first, then the head commit once commits load. A slow branch response
+    // that lands AFTER the commit package is active must not wipe the tree
+    // to "Empty directory".
+    let resolveBranch: ((value: unknown) => void) | null = null;
+    let resolveCommit: ((value: unknown) => void) | null = null;
+    global.fetch = jest.fn((input: unknown) => {
+      const url = String(input);
+      const params = new URLSearchParams(url.split('?')[1] || '');
+      const ref = params.get('ref') || '';
+      if (ref === 'main') {
+        return new Promise((resolve) => {
+          resolveBranch = resolve;
+        });
+      }
+      if (ref === 'abc123') {
+        return new Promise((resolve) => {
+          resolveCommit = resolve;
+        });
+      }
+      return Promise.resolve({
+        ok: true,
+        status: 200,
+        json: async () => ({ items: [] }),
+      });
+    }) as unknown as typeof fetch;
+
+    const { rerender } = render(
+      <VCSFileTreePicker
+        provider="github"
+        repositoryFullName="engineeredsoftware/ENGI"
+        treeRef="main"
+        selectedPaths={[]}
+        onChange={jest.fn()}
+      />,
+    );
+
+    // Tree identity hops to the head commit before the branch fetch settles.
+    rerender(
+      <VCSFileTreePicker
+        provider="github"
+        repositoryFullName="engineeredsoftware/ENGI"
+        treeRef="abc123"
+        selectedPaths={[]}
+        onChange={jest.fn()}
+      />,
+    );
+
+    // Commit package wins with real items.
+    expect(resolveCommit).toBeTruthy();
+    resolveCommit!({
+      ok: true,
+      status: 200,
+      json: async () => ({
+        items: [
+          { path: 'src', type: 'tree', sha: 't1' },
+          { path: 'README.md', type: 'blob', sha: 'b1', size: 10 },
+        ],
+      }),
+    });
+    await waitFor(() => expect(screen.getByText('src/')).toBeInTheDocument());
+    expect(screen.getByText('README.md')).toBeInTheDocument();
+    expect(screen.queryByText('Empty directory')).not.toBeInTheDocument();
+
+    // Stale branch response arrives late — empty / failed. Must not overwrite.
+    expect(resolveBranch).toBeTruthy();
+    resolveBranch!({
+      ok: false,
+      status: 404,
+      json: async () => ({ error: 'not found' }),
+    });
+    // Give the stale promise a tick to misbehave if the guard is broken.
+    await new Promise((resolve) => setTimeout(resolve, 0));
+    expect(screen.getByText('src/')).toBeInTheDocument();
+    expect(screen.getByText('README.md')).toBeInTheDocument();
+    expect(screen.queryByText('Empty directory')).not.toBeInTheDocument();
+  });
 });
