From c387be08b96eb4fc7a12b510a09006dee9ea472f Mon Sep 17 00:00:00 2001 From: Gille <4317663+helix4u@users.noreply.github.com> Date: Thu, 16 Jul 2026 13:05:14 -0600 Subject: [PATCH] fix(desktop): serialize git status refreshes (#65341) --- apps/desktop/electron/git-review-ops.test.ts | 48 ++++++++++++- apps/desktop/electron/git-review-ops.ts | 15 ++-- apps/desktop/src/store/coding-status.test.ts | 48 ++++++++++++- apps/desktop/src/store/coding-status.ts | 73 ++++++++++++++------ 4 files changed, 155 insertions(+), 29 deletions(-) diff --git a/apps/desktop/electron/git-review-ops.test.ts b/apps/desktop/electron/git-review-ops.test.ts index a06fcdace39f..a48c067ab1ab 100644 --- a/apps/desktop/electron/git-review-ops.test.ts +++ b/apps/desktop/electron/git-review-ops.test.ts @@ -1,8 +1,34 @@ import assert from 'node:assert/strict' +import { execFileSync } from 'node:child_process' +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' -import { test } from 'vitest' +import { afterEach, test } from 'vitest' -import { resolveRenamePath } from './git-review-ops' +import { repoStatus, resolveRenamePath } from './git-review-ops' + +const tempDirs: string[] = [] + +afterEach(() => { + for (const dir of tempDirs.splice(0)) { + fs.rmSync(dir, { force: true, recursive: true }) + } +}) + +function makeRepo() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'hermes-desktop-git-status-')) + + tempDirs.push(dir) + execFileSync('git', ['init', '-q'], { cwd: dir }) + execFileSync('git', ['config', 'user.email', 'hermes-test@example.com'], { cwd: dir }) + execFileSync('git', ['config', 'user.name', 'Hermes Test'], { cwd: dir }) + fs.writeFileSync(path.join(dir, 'tracked.txt'), 'tracked\n') + execFileSync('git', ['add', 'tracked.txt'], { cwd: dir }) + execFileSync('git', ['commit', '-qm', 'initial'], { cwd: dir }) + + return dir +} test('resolveRenamePath: plain path is unchanged', () => { assert.equal(resolveRenamePath('src/a.ts'), 'src/a.ts') @@ -19,3 +45,21 @@ test('resolveRenamePath: brace rename resolves to the new path', () => { test('resolveRenamePath: brace rename collapsing a segment', () => { assert.equal(resolveRenamePath('src/{lib => }/file.ts'), 'src/file.ts') }) + +test('repoStatus reports an untracked directory without recursively listing its contents', async () => { + const dir = makeRepo() + const nested = path.join(dir, 'generated', 'deep') + + fs.mkdirSync(nested, { recursive: true }) + fs.writeFileSync(path.join(nested, 'large-output.txt'), 'generated\n') + + const status = await repoStatus(dir, 'git') + + assert.ok(status) + assert.equal(status.untracked, 1) + assert.equal(status.changed, 1) + assert.deepEqual( + status.files.map(file => file.path), + ['generated/'] + ) +}) diff --git a/apps/desktop/electron/git-review-ops.ts b/apps/desktop/electron/git-review-ops.ts index 1baf26d2fca3..472e246ccf01 100644 --- a/apps/desktop/electron/git-review-ops.ts +++ b/apps/desktop/electron/git-review-ops.ts @@ -610,7 +610,11 @@ async function repoStatus(repoPath, gitBin) { let status try { - status = await git.status() + // The coding rail needs compact change truth, not every generated file. + // `simple-git` defaults bare `-u` to recursive `all`, which can make a + // generated workspace consume gigabytes before the 200-row UI cap is + // applied. `normal` reports each untracked directory as one entry. + status = await git.status(['--untracked-files=normal']) } catch { // Not a repo / git unavailable / remote backend. return null @@ -652,11 +656,10 @@ async function repoStatus(repoPath, gitBin) { } // `git diff HEAD` ignores untracked files, so a turn that only creates new - // files (the common case — a fresh module, a demo dir) showed +0 in the rail - // while the review pane counted them. Fold untracked insertions into `added` - // so the rail matches reality. Bounded (size cap + concurrency) like the - // review tree; only the capped file slice is counted so a huge untracked tree - // can't stall the probe. + // files (the common case — a fresh module) showed +0 in the rail while the + // review pane counted them. Fold top-level untracked file insertions into + // `added`; directories reported by the compact `normal` scan intentionally + // remain at zero rather than recursively walking their contents. try { const untracked = status.not_added.slice(0, 500) diff --git a/apps/desktop/src/store/coding-status.test.ts b/apps/desktop/src/store/coding-status.test.ts index 02d9e843bb65..d83e31a4849e 100644 --- a/apps/desktop/src/store/coding-status.test.ts +++ b/apps/desktop/src/store/coding-status.test.ts @@ -2,7 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { HermesRepoStatus } from '@/global' -import { $repoStatus, refreshRepoStatus } from './coding-status' +import { $repoStatus, $repoStatusLoading, refreshRepoStatus } from './coding-status' import { $currentCwd } from './session' const sampleStatus: HermesRepoStatus = { @@ -27,12 +27,15 @@ function stubProbe(impl: (cwd: string) => Promise) { describe('refreshRepoStatus', () => { beforeEach(() => { + vi.useFakeTimers() $repoStatus.set(null) $currentCwd.set('') delete (window as unknown as { hermesDesktop?: unknown }).hermesDesktop }) afterEach(() => { + vi.clearAllTimers() + vi.useRealTimers() delete (window as unknown as { hermesDesktop?: unknown }).hermesDesktop }) @@ -71,4 +74,47 @@ describe('refreshRepoStatus', () => { await refreshRepoStatus('/repo') expect($repoStatus.get()).toBeNull() }) + + it('runs one probe at a time and coalesces overlap into one trailing refresh', async () => { + const resolvers: Array<(status: HermesRepoStatus | null) => void> = [] + const calls: string[] = [] + let active = 0 + let maxActive = 0 + + stubProbe( + cwd => + new Promise(resolve => { + calls.push(cwd) + active++ + maxActive = Math.max(maxActive, active) + resolvers.push(status => { + active-- + resolve(status) + }) + }) + ) + + const first = refreshRepoStatus('/repo-a') + const second = refreshRepoStatus('/repo-b') + const third = refreshRepoStatus('/repo-c') + + expect(calls).toEqual(['/repo-a']) + expect(maxActive).toBe(1) + expect($repoStatusLoading.get()).toBe(true) + + resolvers.shift()?.(sampleStatus) + await Promise.resolve() + await Promise.resolve() + + expect(calls).toEqual(['/repo-a', '/repo-c']) + expect(maxActive).toBe(1) + expect($repoStatus.get()).toBeNull() + + resolvers.shift()?.(sampleStatus) + await Promise.all([first, second, third]) + + expect(maxActive).toBe(1) + expect($repoStatus.get()).toEqual(sampleStatus) + expect($repoStatusLoading.get()).toBe(false) + }) }) diff --git a/apps/desktop/src/store/coding-status.ts b/apps/desktop/src/store/coding-status.ts index bd353e0d78a2..84f9c9b2aea5 100644 --- a/apps/desktop/src/store/coding-status.ts +++ b/apps/desktop/src/store/coding-status.ts @@ -66,9 +66,19 @@ async function loadWorktrees(target: string): Promise { } } +interface RepoStatusRefreshRequest { + probe: (cwd: string) => Promise + seq: number + target: string +} + // Coalesce overlapping probes: many triggers can fire around a turn boundary -// (busy flip + worktree token + focus), but only the latest cwd matters. +// (busy flip + worktree token + focus), but only the latest cwd matters. Keep +// one probe in flight and retain at most one trailing request so a slow Git +// status cannot multiply into an unbounded subprocess pile-up. let inflightCwd: null | string = null +let pendingRepoStatusRefresh: RepoStatusRefreshRequest | null = null +let repoStatusRefreshInFlight: Promise | null = null let repoStatusRefreshSeq = 0 let repoStatusRefreshTimer: ReturnType | null = null @@ -79,21 +89,7 @@ const normalizeCwd = (cwd?: null | string): null | string => cwd?.trim() || null * Best-effort: a non-repo, a remote backend, or a missing probe clears the * status so the rail hides rather than showing stale data. */ -export async function refreshRepoStatus(cwd?: null | string): Promise { - const target = normalizeCwd(cwd ?? $currentCwd.get()) - const probe = desktopGit()?.repoStatus - const seq = (repoStatusRefreshSeq += 1) - - if (!target || !probe) { - $repoStatus.set(null) - $repoWorktrees.set([]) - - return - } - - inflightCwd = target - $repoStatusLoading.set(true) - +async function runRepoStatusRefresh({ probe, seq, target }: RepoStatusRefreshRequest): Promise { try { const status = await probe(target) @@ -114,13 +110,50 @@ export async function refreshRepoStatus(cwd?: null | string): Promise { $repoStatus.set(null) $repoWorktrees.set([]) } - } finally { - if (seq === repoStatusRefreshSeq && inflightCwd === target) { - $repoStatusLoading.set(false) - } } } +async function drainRepoStatusRefreshes(): Promise { + while (pendingRepoStatusRefresh) { + const request = pendingRepoStatusRefresh + + pendingRepoStatusRefresh = null + await runRepoStatusRefresh(request) + } + + // This reset is synchronous with the final empty-queue check. A refresh + // arriving before this continuation runs is drained above; one arriving + // afterward sees no in-flight promise and starts a new drain. + repoStatusRefreshInFlight = null + $repoStatusLoading.set(false) +} + +export function refreshRepoStatus(cwd?: null | string): Promise { + const target = normalizeCwd(cwd ?? $currentCwd.get()) + const probe = desktopGit()?.repoStatus + const seq = (repoStatusRefreshSeq += 1) + + if (!target || !probe) { + pendingRepoStatusRefresh = null + inflightCwd = null + $repoStatus.set(null) + $repoWorktrees.set([]) + $repoStatusLoading.set(false) + + return repoStatusRefreshInFlight || Promise.resolve() + } + + inflightCwd = target + pendingRepoStatusRefresh = { probe, seq, target } + $repoStatusLoading.set(true) + + if (!repoStatusRefreshInFlight) { + repoStatusRefreshInFlight = drainRepoStatusRefreshes() + } + + return repoStatusRefreshInFlight +} + function scheduleRepoStatusRefresh(cwd?: null | string): void { if (repoStatusRefreshTimer) { clearTimeout(repoStatusRefreshTimer)