fix(git): make post-mutation status refresh authoritative (#2740)
fix(git): make post-mutation status refresh authoritative
This commit is contained in:
@@ -147,10 +147,12 @@ Important properties:
|
||||
- `directories: Map<string, DirectoryGitState>` is the source of truth
|
||||
- loading state is per-directory, not global
|
||||
- `ensureStatus()` and `ensureAll()` are the preferred entry points for consumers
|
||||
- in-flight dedupe exists for status and `ensureAll()`
|
||||
- in-flight dedupe exists for status and `ensureAll()`; status dedupe is scoped to the per-directory status mutation revision, so a refresh requested after a mutation never joins a pre-mutation in-flight request
|
||||
- runtime reset replaces all live entries with that runtime's persisted branch seeds and invalidates old completions
|
||||
- status, branches, log, identity, repository probes, and prefetch diffs commit through runtime and per-channel generations
|
||||
- status mutations advance a revision so older refreshes cannot undo optimistic or confirmed index changes
|
||||
- a successful status-affecting git mutation also advances that revision: the HTTP adapter's cache invalidation notifies the store through `lib/gitStatusInvalidation.ts` (the VS Code bridge adapter has no client-side status cache, so it emits nothing today)
|
||||
- `fetchAll({ force: true })` forces the status fetch as well as the log refresh
|
||||
- branch persistence is versioned, bounded, runtime-scoped, and claims the ambiguous legacy cache once
|
||||
- diff data has per-directory and aggregate count/UTF-8-byte limits; oversized single entries are rejected
|
||||
|
||||
@@ -312,6 +314,7 @@ Expected model:
|
||||
|
||||
- `GitView` / `DiffView` ensure current-directory Git state when visible
|
||||
- explicit Git actions refresh status/branches/log as needed
|
||||
- every status-affecting git mutation invalidates the HTTP adapter's status cache on its success path (failed mutations invalidate nothing), so the follow-up refresh is authoritative instead of the pre-mutation cache entry
|
||||
- a mounted file-mutating tool issues a one-shot Git refresh hint when it transitions from active to successfully finalized; remounting historical completed tools does not replay the hint
|
||||
- a successful dirty save from the in-app file editor issues a path-scoped Git refresh hint; clean autosave checks remain no-ops
|
||||
- refresh hints with authoritative file paths invalidate only those cached and currently rendered diffs before status refresh; pathless tools request status reconciliation without broadly remounting DiffView
|
||||
|
||||
@@ -2,6 +2,7 @@ import { beforeEach, describe, expect, test } from 'bun:test';
|
||||
import type { GitStatus } from '@/lib/api/types';
|
||||
import { useGitStore } from './useGitStore';
|
||||
import { getRuntimeKey } from '@/lib/runtime-switch';
|
||||
import { notifyGitStatusInvalidated } from '@/lib/gitStatusInvalidation';
|
||||
|
||||
type Deferred<T> = {
|
||||
promise: Promise<T>;
|
||||
@@ -126,6 +127,85 @@ describe('useGitStore', () => {
|
||||
expect(lightResult).toBe(fullResult);
|
||||
});
|
||||
|
||||
test('deduplicates concurrent status requests when no mutation occurs', async () => {
|
||||
setDirectoryStatus(createStatus());
|
||||
let statusCalls = 0;
|
||||
const request = createDeferred<GitStatus>();
|
||||
const git = createGitApi(() => {
|
||||
statusCalls += 1;
|
||||
return request.promise;
|
||||
});
|
||||
|
||||
const first = useGitStore.getState().fetchStatus('/repo', git, { silent: true });
|
||||
const second = useGitStore.getState().fetchStatus('/repo', git, { silent: true });
|
||||
await Promise.resolve();
|
||||
|
||||
expect(statusCalls).toBe(1);
|
||||
|
||||
request.resolve(createStatus());
|
||||
await Promise.all([first, second]);
|
||||
expect(statusCalls).toBe(1);
|
||||
});
|
||||
|
||||
test('a refresh after a mutation does not join the pre-mutation in-flight status request', async () => {
|
||||
setDirectoryStatus(createStatus());
|
||||
const requests: Deferred<GitStatus>[] = [];
|
||||
let statusCalls = 0;
|
||||
const git = createGitApi(() => {
|
||||
statusCalls += 1;
|
||||
const request = createDeferred<GitStatus>();
|
||||
requests.push(request);
|
||||
return request.promise;
|
||||
});
|
||||
|
||||
const preMutation = useGitStore.getState().fetchStatus('/repo', git, { silent: true });
|
||||
await Promise.resolve();
|
||||
expect(statusCalls).toBe(1);
|
||||
|
||||
// A successful git mutation invalidates the adapter status cache, which
|
||||
// notifies the store that the in-flight request predates the mutation.
|
||||
notifyGitStatusInvalidated('/repo');
|
||||
|
||||
const postMutation = useGitStore.getState().fetchStatus('/repo', git, { silent: true });
|
||||
await Promise.resolve();
|
||||
expect(statusCalls).toBe(2);
|
||||
|
||||
requests[1].resolve({ ...createStatus(), current: 'feature' });
|
||||
await postMutation;
|
||||
expect(useGitStore.getState().getDirectoryState('/repo')?.status?.current).toBe('feature');
|
||||
|
||||
// The late pre-mutation response cannot overwrite the newer authoritative one.
|
||||
requests[0].resolve(createStatus());
|
||||
await preMutation;
|
||||
expect(useGitStore.getState().getDirectoryState('/repo')?.status?.current).toBe('feature');
|
||||
});
|
||||
|
||||
test('fetchAll({ force: true }) forces a fresh status fetch past the in-flight dedup', async () => {
|
||||
setDirectoryStatus(createStatus());
|
||||
const requests: Deferred<GitStatus>[] = [];
|
||||
let statusCalls = 0;
|
||||
const git = createGitApi(() => {
|
||||
statusCalls += 1;
|
||||
const request = createDeferred<GitStatus>();
|
||||
requests.push(request);
|
||||
return request.promise;
|
||||
});
|
||||
|
||||
const inFlight = useGitStore.getState().fetchStatus('/repo', git, { silent: true });
|
||||
await Promise.resolve();
|
||||
expect(statusCalls).toBe(1);
|
||||
|
||||
const all = useGitStore.getState().fetchAll('/repo', git, { force: true });
|
||||
await Promise.resolve();
|
||||
expect(statusCalls).toBe(2);
|
||||
|
||||
requests[1].resolve({ ...createStatus(), current: 'feature' });
|
||||
requests[0].resolve(createStatus());
|
||||
await Promise.allSettled([inFlight, all]);
|
||||
|
||||
expect(useGitStore.getState().getDirectoryState('/repo')?.status?.current).toBe('feature');
|
||||
});
|
||||
|
||||
test('does not let an older status fetch undo an optimistic mutation', async () => {
|
||||
const initial = createStatus(undefined, [{ path: 'src/index.ts', index: ' ', working_dir: 'M' }]);
|
||||
setDirectoryStatus(initial);
|
||||
|
||||
@@ -9,6 +9,7 @@ import type {
|
||||
} from '@/lib/api/types';
|
||||
import { getDeferredSafeStorage } from '@/stores/utils/safeStorage';
|
||||
import { getRuntimeKey } from '@/lib/runtime-switch';
|
||||
import { subscribeGitStatusInvalidations } from '@/lib/gitStatusInvalidation';
|
||||
|
||||
const LOG_STALE_THRESHOLD = 10000;
|
||||
const REPO_CHECK_STALE_THRESHOLD = 60_000;
|
||||
@@ -57,7 +58,7 @@ interface GitStore {
|
||||
setActiveDirectory: (directory: string | null) => void;
|
||||
getDirectoryState: (directory: string) => DirectoryGitState | null;
|
||||
|
||||
fetchStatus: (directory: string, git: GitAPI, options?: { silent?: boolean; mode?: 'light' }) => Promise<boolean>;
|
||||
fetchStatus: (directory: string, git: GitAPI, options?: { silent?: boolean; mode?: 'light'; force?: boolean }) => Promise<boolean>;
|
||||
fetchBranches: (directory: string, git: GitAPI) => Promise<void>;
|
||||
fetchLog: (directory: string, git: GitAPI, maxCount?: number) => Promise<void>;
|
||||
fetchIdentity: (directory: string, git: GitAPI) => Promise<void>;
|
||||
@@ -99,7 +100,7 @@ interface GitAPI {
|
||||
|
||||
const inFlightDiffFetchesByDirectory = new Map<string, Set<string>>();
|
||||
const diffFetchGenerationByDirectory = new Map<string, number>();
|
||||
const inFlightStatusFetches = new Map<string, Promise<boolean>>();
|
||||
const inFlightStatusFetches = new Map<string, { promise: Promise<boolean>; statusMutationRevision: number }>();
|
||||
const inFlightEnsureAllByDirectory = new Map<string, Promise<void>>();
|
||||
const requestGenerationByChannel = new Map<string, number>();
|
||||
const statusMutationRevisionByDirectory = new Map<string, number>();
|
||||
@@ -150,6 +151,18 @@ const bumpStatusMutationRevision = (runtimeKey: string, directory: string): void
|
||||
statusMutationRevisionByDirectory.set(key, (statusMutationRevisionByDirectory.get(key) ?? 0) + 1);
|
||||
};
|
||||
|
||||
const getStatusMutationRevision = (runtimeKey: string, directory: string): number =>
|
||||
statusMutationRevisionByDirectory.get(runtimeDirectoryKey(runtimeKey, directory)) ?? 0;
|
||||
|
||||
// A successful status-affecting git mutation invalidates the runtime adapter's
|
||||
// status cache (see lib/gitStatusInvalidation.ts). Bump the per-directory
|
||||
// mutation revision so a status request admitted before the mutation can
|
||||
// neither be joined by a post-mutation refresh nor commit its stale payload
|
||||
// over the refreshed state.
|
||||
subscribeGitStatusInvalidations((directory) => {
|
||||
bumpStatusMutationRevision(getRuntimeKey(), directory);
|
||||
});
|
||||
|
||||
const getDiffFetchGeneration = (directory: string): number =>
|
||||
diffFetchGenerationByDirectory.get(runtimeDirectoryKey(getRuntimeKey(), directory)) ?? 0;
|
||||
|
||||
@@ -590,10 +603,16 @@ export const useGitStore = create<GitStore>()(
|
||||
const statusFetchMode: GitStatusFetchMode = options.mode ?? 'full';
|
||||
const runtimeKey = getRuntimeKey();
|
||||
const statusFetchKey = getStatusFetchKey(runtimeKey, directory, statusFetchMode);
|
||||
const existing = inFlightStatusFetches.get(statusFetchKey)
|
||||
?? (statusFetchMode === 'light' ? inFlightStatusFetches.get(getStatusFetchKey(runtimeKey, directory, 'full')) : undefined);
|
||||
if (existing) {
|
||||
return existing;
|
||||
const statusMutationRevision = getStatusMutationRevision(runtimeKey, directory);
|
||||
if (!options.force) {
|
||||
const existing = inFlightStatusFetches.get(statusFetchKey)
|
||||
?? (statusFetchMode === 'light' ? inFlightStatusFetches.get(getStatusFetchKey(runtimeKey, directory, 'full')) : undefined);
|
||||
// Join an in-flight request only when it was admitted at the current
|
||||
// mutation revision; a request that predates a mutation must not
|
||||
// satisfy the post-mutation refresh.
|
||||
if (existing && existing.statusMutationRevision === statusMutationRevision) {
|
||||
return existing.promise;
|
||||
}
|
||||
}
|
||||
|
||||
const token = startRequest(directory, 'status', true);
|
||||
@@ -727,12 +746,12 @@ export const useGitStore = create<GitStore>()(
|
||||
return statusChanged;
|
||||
})();
|
||||
|
||||
inFlightStatusFetches.set(statusFetchKey, fetchPromise);
|
||||
inFlightStatusFetches.set(statusFetchKey, { promise: fetchPromise, statusMutationRevision });
|
||||
|
||||
try {
|
||||
return await fetchPromise;
|
||||
} finally {
|
||||
if (inFlightStatusFetches.get(statusFetchKey) === fetchPromise) {
|
||||
if (inFlightStatusFetches.get(statusFetchKey)?.promise === fetchPromise) {
|
||||
inFlightStatusFetches.delete(statusFetchKey);
|
||||
}
|
||||
}
|
||||
@@ -936,8 +955,11 @@ export const useGitStore = create<GitStore>()(
|
||||
const { force = false, silentIfCached = false } = options;
|
||||
const now = Date.now();
|
||||
|
||||
// `force` applies to status as well as log: a forced refresh must not
|
||||
// resolve from an in-flight status request admitted earlier.
|
||||
await get().fetchStatus(directory, git, {
|
||||
silent: silentIfCached && Boolean(dirState?.status),
|
||||
force,
|
||||
});
|
||||
|
||||
const updatedDirState = get().directories.get(directory);
|
||||
|
||||
Reference in New Issue
Block a user