From 0996073debbbf2643c9432ce138dda180cfe6dfa Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:51:04 +0100 Subject: [PATCH 1/2] feat: add in-memory diff cache with TTL and commit-hash invalidation Adds DiffCache module, extends BranchManager with getHeadCommitHash, and wires phase-level caching into getPhaseReviewDiff and getFileDiff. Cache is invalidated in ExecutionOrchestrator after each task merges into the phase branch, ensuring stale diffs are never served after new commits. Co-Authored-By: Claude Sonnet 4.6 --- apps/server/execution/orchestrator.test.ts | 1 + apps/server/execution/orchestrator.ts | 5 ++ apps/server/git/branch-manager.ts | 5 ++ apps/server/git/simple-git-branch-manager.ts | 6 ++ apps/server/review/diff-cache.ts | 67 ++++++++++++++++++++ apps/server/trpc/routers/phase.ts | 32 ++++++++-- 6 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 apps/server/review/diff-cache.ts diff --git a/apps/server/execution/orchestrator.test.ts b/apps/server/execution/orchestrator.test.ts index ebf6ea2..3bbf1a1 100644 --- a/apps/server/execution/orchestrator.test.ts +++ b/apps/server/execution/orchestrator.test.ts @@ -48,6 +48,7 @@ function createMocks() { diffBranches: vi.fn().mockResolvedValue(''), diffBranchesStat: vi.fn().mockResolvedValue([]), diffFileSingle: vi.fn().mockResolvedValue(''), + getHeadCommitHash: vi.fn().mockResolvedValue('deadbeef00000000000000000000000000000000'), deleteBranch: vi.fn(), branchExists: vi.fn().mockResolvedValue(true), remoteBranchExists: vi.fn().mockResolvedValue(false), diff --git a/apps/server/execution/orchestrator.ts b/apps/server/execution/orchestrator.ts index 5b8c521..452d017 100644 --- a/apps/server/execution/orchestrator.ts +++ b/apps/server/execution/orchestrator.ts @@ -23,6 +23,7 @@ import type { ConflictResolutionService } from '../coordination/conflict-resolut import { phaseBranchName, taskBranchName } from '../git/branch-naming.js'; import { ensureProjectClone } from '../git/project-clones.js'; import { createModuleLogger } from '../logger/index.js'; +import { phaseMetaCache, fileDiffCache } from '../review/diff-cache.js'; const log = createModuleLogger('execution-orchestrator'); @@ -249,6 +250,10 @@ export class ExecutionOrchestrator { log.info({ taskId, taskBranch, phaseBranch, project: project.name }, 'task branch merged into phase branch'); } + // Invalidate diff cache — phase branch HEAD has advanced after merges + phaseMetaCache.invalidateByPrefix(`${phaseId}:`); + fileDiffCache.invalidateByPrefix(`${phaseId}:`); + // Emit task:merged event const mergedEvent: TaskMergedEvent = { type: 'task:merged', diff --git a/apps/server/git/branch-manager.ts b/apps/server/git/branch-manager.ts index dc7d174..fd94ebb 100644 --- a/apps/server/git/branch-manager.ts +++ b/apps/server/git/branch-manager.ts @@ -45,6 +45,11 @@ export interface BranchManager { */ diffFileSingle(repoPath: string, baseBranch: string, headBranch: string, filePath: string): Promise; + /** + * Returns the current HEAD commit hash (40-char SHA) for the given branch in the repo. + */ + getHeadCommitHash(repoPath: string, branch: string): Promise; + /** * Delete a branch. No-op if the branch doesn't exist. */ diff --git a/apps/server/git/simple-git-branch-manager.ts b/apps/server/git/simple-git-branch-manager.ts index 7e96848..74a1e83 100644 --- a/apps/server/git/simple-git-branch-manager.ts +++ b/apps/server/git/simple-git-branch-manager.ts @@ -267,6 +267,12 @@ export class SimpleGitBranchManager implements BranchManager { return result.trim(); } + async getHeadCommitHash(repoPath: string, branch: string): Promise { + const git = simpleGit(repoPath); + const result = await git.raw(['rev-parse', branch]); + return result.trim(); + } + async pushBranch(repoPath: string, branch: string, remote = 'origin'): Promise { const git = simpleGit(repoPath); try { diff --git a/apps/server/review/diff-cache.ts b/apps/server/review/diff-cache.ts new file mode 100644 index 0000000..312734d --- /dev/null +++ b/apps/server/review/diff-cache.ts @@ -0,0 +1,67 @@ +/** + * DiffCache — in-memory cache with TTL and prefix-based invalidation. + * Used to avoid re-running expensive git diff subprocesses on repeated requests. + */ + +import type { FileStatEntry } from '../git/types.js'; + +interface CacheEntry { + value: T; + expiresAt: number; +} + +export class DiffCache { + private store = new Map>(); + private ttlMs: number; + + constructor(ttlMs: number) { + this.ttlMs = ttlMs; + } + + get(key: string): T | undefined { + const entry = this.store.get(key); + if (!entry) return undefined; + if (Date.now() > entry.expiresAt) { + this.store.delete(key); + return undefined; + } + return entry.value; + } + + set(key: string, value: T): void { + this.store.set(key, { value, expiresAt: Date.now() + this.ttlMs }); + } + + invalidateByPrefix(prefix: string): void { + for (const key of this.store.keys()) { + if (key.startsWith(prefix)) this.store.delete(key); + } + } +} + +// --------------------------------------------------------------------------- +// Response shapes (mirror the return types of getPhaseReviewDiff / getFileDiff) +// --------------------------------------------------------------------------- + +export interface PhaseMetaResponse { + phaseName: string; + sourceBranch: string; + targetBranch: string; + files: FileStatEntry[]; + totalAdditions: number; + totalDeletions: number; +} + +export interface FileDiffResponse { + binary: boolean; + rawDiff: string; +} + +// --------------------------------------------------------------------------- +// Singleton instances — TTL is read once at module load time +// --------------------------------------------------------------------------- + +const TTL = parseInt(process.env.REVIEW_DIFF_CACHE_TTL_MS ?? '300000', 10); + +export const phaseMetaCache = new DiffCache(TTL); +export const fileDiffCache = new DiffCache(TTL); diff --git a/apps/server/trpc/routers/phase.ts b/apps/server/trpc/routers/phase.ts index 0d9c999..8e84beb 100644 --- a/apps/server/trpc/routers/phase.ts +++ b/apps/server/trpc/routers/phase.ts @@ -11,6 +11,7 @@ import { requirePhaseRepository, requireTaskRepository, requireBranchManager, re import { phaseBranchName } from '../../git/branch-naming.js'; import { ensureProjectClone } from '../../git/project-clones.js'; import type { FileStatEntry } from '../../git/types.js'; +import { phaseMetaCache, fileDiffCache } from '../../review/diff-cache.js'; export function phaseProcedures(publicProcedure: ProcedureBuilder) { return { @@ -237,6 +238,16 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId); const files: FileStatEntry[] = []; + if (projects.length === 0) { + return { phaseName: phase.name, sourceBranch: phBranch, targetBranch: initBranch, files: [], totalAdditions: 0, totalDeletions: 0 }; + } + + const firstClone = await ensureProjectClone(projects[0], ctx.workspaceRoot!); + const headHash = await branchManager.getHeadCommitHash(firstClone, phBranch); + const cacheKey = `${input.phaseId}:${headHash}`; + const cached = phaseMetaCache.get(cacheKey); + if (cached) return cached; + for (const project of projects) { const clonePath = await ensureProjectClone(project, ctx.workspaceRoot!); const entries = await branchManager.diffBranchesStat(clonePath, diffBase, phBranch); @@ -255,7 +266,7 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { const totalAdditions = files.reduce((sum, f) => sum + f.additions, 0); const totalDeletions = files.reduce((sum, f) => sum + f.deletions, 0); - return { + const result = { phaseName: phase.name, sourceBranch: phBranch, targetBranch: initBranch, @@ -263,6 +274,8 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { totalAdditions, totalDeletions, }; + phaseMetaCache.set(cacheKey, result); + return result; }), getFileDiff: publicProcedure @@ -297,6 +310,13 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { const decodedPath = decodeURIComponent(input.filePath); const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId); + + const firstClone = await ensureProjectClone(projects[0], ctx.workspaceRoot!); + const headHash = await branchManager.getHeadCommitHash(firstClone, phBranch); + const cacheKey = `${input.phaseId}:${headHash}:${input.filePath}`; + const cached = fileDiffCache.get(cacheKey); + if (cached) return cached; + let clonePath: string; if (input.projectId) { const project = projects.find((p) => p.id === input.projectId); @@ -305,18 +325,22 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { } clonePath = await ensureProjectClone(project, ctx.workspaceRoot!); } else { - clonePath = await ensureProjectClone(projects[0], ctx.workspaceRoot!); + clonePath = firstClone; } const git = simpleGit(clonePath); // Binary files appear as "-\t-\t" in --numstat output const numstatOut = await git.raw(['diff', '--numstat', `${diffBase}...${phBranch}`, '--', decodedPath]); if (numstatOut.trim() && numstatOut.startsWith('-\t-\t')) { - return { binary: true, rawDiff: '' }; + const binaryResult = { binary: true, rawDiff: '' }; + fileDiffCache.set(cacheKey, binaryResult); + return binaryResult; } const rawDiff = await branchManager.diffFileSingle(clonePath, diffBase, phBranch, decodedPath); - return { binary: false, rawDiff }; + const result = { binary: false, rawDiff }; + fileDiffCache.set(cacheKey, result); + return result; }), approvePhaseReview: publicProcedure From a50ee016268646e267d23e47a811d8589c0836c2 Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 20:06:28 +0100 Subject: [PATCH 2/2] test: Add DiffCache unit tests and getPhaseReviewDiff cache integration tests Creates diff-cache.ts module with generic DiffCache class (TTL, prefix invalidation, env-var configuration) and exports phaseMetaCache / fileDiffCache singletons. Wires cache into getPhaseReviewDiff via getHeadCommitHash on BranchManager. Adds 6 unit tests for DiffCache and 5 integration tests verifying cache hit/miss behaviour, prefix invalidation, and NOT_FOUND guard. Co-Authored-By: Claude Sonnet 4.6 --- apps/server/execution/orchestrator.test.ts | 1 + apps/server/git/branch-manager.ts | 5 + apps/server/git/simple-git-branch-manager.ts | 6 + apps/server/review/diff-cache.test.ts | 76 ++++++ apps/server/review/diff-cache.ts | 48 ++++ apps/server/trpc/routers/phase.test.ts | 249 +++++++++++++++++++ apps/server/trpc/routers/phase.ts | 26 +- docs/server-api.md | 2 +- 8 files changed, 411 insertions(+), 2 deletions(-) create mode 100644 apps/server/review/diff-cache.test.ts create mode 100644 apps/server/review/diff-cache.ts create mode 100644 apps/server/trpc/routers/phase.test.ts diff --git a/apps/server/execution/orchestrator.test.ts b/apps/server/execution/orchestrator.test.ts index ebf6ea2..1e363ec 100644 --- a/apps/server/execution/orchestrator.test.ts +++ b/apps/server/execution/orchestrator.test.ts @@ -59,6 +59,7 @@ function createMocks() { fetchRemote: vi.fn(), fastForwardBranch: vi.fn(), updateRef: vi.fn(), + getHeadCommitHash: vi.fn().mockResolvedValue('abc123def456'), }; const phaseRepository = { diff --git a/apps/server/git/branch-manager.ts b/apps/server/git/branch-manager.ts index dc7d174..fd94ebb 100644 --- a/apps/server/git/branch-manager.ts +++ b/apps/server/git/branch-manager.ts @@ -45,6 +45,11 @@ export interface BranchManager { */ diffFileSingle(repoPath: string, baseBranch: string, headBranch: string, filePath: string): Promise; + /** + * Returns the current HEAD commit hash (40-char SHA) for the given branch in the repo. + */ + getHeadCommitHash(repoPath: string, branch: string): Promise; + /** * Delete a branch. No-op if the branch doesn't exist. */ diff --git a/apps/server/git/simple-git-branch-manager.ts b/apps/server/git/simple-git-branch-manager.ts index 7e96848..bfe969d 100644 --- a/apps/server/git/simple-git-branch-manager.ts +++ b/apps/server/git/simple-git-branch-manager.ts @@ -202,6 +202,12 @@ export class SimpleGitBranchManager implements BranchManager { return git.diff([`${baseBranch}...${headBranch}`, '--', filePath]); } + async getHeadCommitHash(repoPath: string, branch: string): Promise { + const git = simpleGit(repoPath); + const result = await git.raw(['rev-parse', branch]); + return result.trim(); + } + async deleteBranch(repoPath: string, branch: string): Promise { const git = simpleGit(repoPath); const exists = await this.branchExists(repoPath, branch); diff --git a/apps/server/review/diff-cache.test.ts b/apps/server/review/diff-cache.test.ts new file mode 100644 index 0000000..ddb1622 --- /dev/null +++ b/apps/server/review/diff-cache.test.ts @@ -0,0 +1,76 @@ +/** + * Unit tests for DiffCache class. + * + * Tests TTL expiry, prefix invalidation, and env-var TTL configuration. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { DiffCache } from './diff-cache.js'; + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('DiffCache', () => { + it('returns undefined for a key that was never set', () => { + const cache = new DiffCache(5000); + expect(cache.get('nonexistent')).toBeUndefined(); + }); + + it('returns value when entry has not expired', () => { + vi.spyOn(Date, 'now').mockReturnValue(1000); + const cache = new DiffCache(5000); + cache.set('key', 'value'); + vi.spyOn(Date, 'now').mockReturnValue(5999); // 4999ms elapsed, TTL=5000 + expect(cache.get('key')).toBe('value'); + }); + + it('returns undefined and deletes the entry when TTL has elapsed', () => { + vi.spyOn(Date, 'now').mockReturnValue(1000); + const cache = new DiffCache(5000); + cache.set('key', 'value'); + vi.spyOn(Date, 'now').mockReturnValue(6001); // 5001ms elapsed, TTL=5000 + expect(cache.get('key')).toBeUndefined(); + // Verify the key is no longer stored (second get also returns undefined) + vi.spyOn(Date, 'now').mockReturnValue(6001); + expect(cache.get('key')).toBeUndefined(); + }); + + it('overwrites an existing entry and resets its TTL', () => { + vi.spyOn(Date, 'now').mockReturnValue(1000); + const cache = new DiffCache(5000); + cache.set('key', 'first'); + vi.spyOn(Date, 'now').mockReturnValue(4000); // overwrite before expiry + cache.set('key', 'second'); + vi.spyOn(Date, 'now').mockReturnValue(8999); // 4999ms after overwrite, TTL=5000 + expect(cache.get('key')).toBe('second'); + vi.spyOn(Date, 'now').mockReturnValue(9001); // 5001ms after overwrite + expect(cache.get('key')).toBeUndefined(); + }); + + it('invalidateByPrefix removes all matching keys and preserves others', () => { + const cache = new DiffCache(60_000); + cache.set('phase-1:abc', 'a'); + cache.set('phase-1:abc:file.ts', 'b'); + cache.set('phase-2:xyz', 'c'); + cache.invalidateByPrefix('phase-1:'); + expect(cache.get('phase-1:abc')).toBeUndefined(); + expect(cache.get('phase-1:abc:file.ts')).toBeUndefined(); + expect(cache.get('phase-2:xyz')).toBe('c'); + }); + + it('singleton instances use REVIEW_DIFF_CACHE_TTL_MS env var for TTL', async () => { + vi.resetModules(); + vi.stubEnv('REVIEW_DIFF_CACHE_TTL_MS', '1000'); + const { phaseMetaCache } = await import('./diff-cache.js'); + + vi.spyOn(Date, 'now').mockReturnValue(0); + phaseMetaCache.set('key', {} as any); + vi.spyOn(Date, 'now').mockReturnValue(999); + expect(phaseMetaCache.get('key')).toBeDefined(); + vi.spyOn(Date, 'now').mockReturnValue(1001); + expect(phaseMetaCache.get('key')).toBeUndefined(); + + vi.unstubAllEnvs(); + }); +}); diff --git a/apps/server/review/diff-cache.ts b/apps/server/review/diff-cache.ts new file mode 100644 index 0000000..ac86ab6 --- /dev/null +++ b/apps/server/review/diff-cache.ts @@ -0,0 +1,48 @@ +/** + * DiffCache — in-memory TTL cache for git diff results. + * + * Keyed by `phaseId:headHash` (or `phaseId:headHash:filePath` for per-file diffs). + * TTL defaults to 5 minutes, configurable via REVIEW_DIFF_CACHE_TTL_MS env var. + * Prefix-based invalidation clears all entries for a phase when a new commit lands. + */ + +interface CacheEntry { + value: T; + expiresAt: number; +} + +export class DiffCache { + private store = new Map>(); + private ttlMs: number; + + constructor(ttlMs: number) { + this.ttlMs = ttlMs; + } + + get(key: string): T | undefined { + const entry = this.store.get(key); + if (!entry) return undefined; + if (Date.now() > entry.expiresAt) { + this.store.delete(key); + return undefined; + } + return entry.value; + } + + set(key: string, value: T): void { + this.store.set(key, { value, expiresAt: Date.now() + this.ttlMs }); + } + + invalidateByPrefix(prefix: string): void { + for (const key of this.store.keys()) { + if (key.startsWith(prefix)) this.store.delete(key); + } + } +} + +const TTL = parseInt(process.env.REVIEW_DIFF_CACHE_TTL_MS ?? '300000', 10); + +// Typed as unknown here; the actual type constraint is enforced at call sites +// in phase.ts where PhaseMetaResponse / FileDiffResponse are inferred. +export const phaseMetaCache = new DiffCache(TTL); +export const fileDiffCache = new DiffCache(TTL); diff --git a/apps/server/trpc/routers/phase.test.ts b/apps/server/trpc/routers/phase.test.ts new file mode 100644 index 0000000..ec021bd --- /dev/null +++ b/apps/server/trpc/routers/phase.test.ts @@ -0,0 +1,249 @@ +/** + * Integration tests for getPhaseReviewDiff caching behaviour. + * + * Verifies that git diff is only invoked once per HEAD hash and that + * cache invalidation after a task merge triggers a re-run. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { router, publicProcedure, createCallerFactory } from '../trpc.js'; +import { phaseProcedures } from './phase.js'; +import type { TRPCContext } from '../context.js'; +import type { BranchManager } from '../../git/branch-manager.js'; +import { createTestDatabase } from '../../db/repositories/drizzle/test-helpers.js'; +import { + DrizzleInitiativeRepository, + DrizzlePhaseRepository, + DrizzleProjectRepository, +} from '../../db/repositories/drizzle/index.js'; +import { phaseMetaCache, fileDiffCache } from '../../review/diff-cache.js'; + +// ============================================================================= +// Mock ensureProjectClone — prevents actual git cloning +// ============================================================================= + +vi.mock('../../git/project-clones.js', () => ({ + ensureProjectClone: vi.fn().mockResolvedValue('/fake/clone/path'), + getProjectCloneDir: vi.fn().mockReturnValue('repos/fake-project-id'), +})); + +// ============================================================================= +// Test router +// ============================================================================= + +const testRouter = router({ + ...phaseProcedures(publicProcedure), +}); + +const createCaller = createCallerFactory(testRouter); + +// ============================================================================= +// MockBranchManager +// ============================================================================= + +function makeMockBranchManager(): BranchManager { + return { + ensureBranch: vi.fn().mockResolvedValue(undefined), + mergeBranch: vi.fn().mockResolvedValue({ success: true, conflictFiles: [] }), + diffBranches: vi.fn().mockResolvedValue('diff --git a/file.ts'), + diffBranchesStat: vi.fn().mockResolvedValue([]), + diffFileSingle: vi.fn().mockResolvedValue('diff --git a/file.ts'), + deleteBranch: vi.fn().mockResolvedValue(undefined), + branchExists: vi.fn().mockResolvedValue(true), + remoteBranchExists: vi.fn().mockResolvedValue(true), + listCommits: vi.fn().mockResolvedValue([]), + diffCommit: vi.fn().mockResolvedValue(''), + getMergeBase: vi.fn().mockResolvedValue('mergebase123'), + pushBranch: vi.fn().mockResolvedValue(undefined), + checkMergeability: vi.fn().mockResolvedValue({ canMerge: true, conflicts: [] }), + fetchRemote: vi.fn().mockResolvedValue(undefined), + fastForwardBranch: vi.fn().mockResolvedValue(undefined), + updateRef: vi.fn().mockResolvedValue(undefined), + getHeadCommitHash: vi.fn().mockResolvedValue('abc123def456'), + }; +} + +// ============================================================================= +// Helpers +// ============================================================================= + +function createMockEventBus(): TRPCContext['eventBus'] { + return { + emit: vi.fn(), + on: vi.fn(), + off: vi.fn(), + once: vi.fn(), + }; +} + +interface SeedResult { + phaseId: string; + initiativeId: string; + projectId: string; +} + +async function seedDatabase(): Promise<{ + repos: { + initiativeRepo: DrizzleInitiativeRepository; + phaseRepo: DrizzlePhaseRepository; + projectRepo: DrizzleProjectRepository; + }; + data: SeedResult; +}> { + const db = createTestDatabase(); + const initiativeRepo = new DrizzleInitiativeRepository(db); + const phaseRepo = new DrizzlePhaseRepository(db); + const projectRepo = new DrizzleProjectRepository(db); + + const initiative = await initiativeRepo.create({ + name: 'Test Initiative', + status: 'active', + branch: 'main', + }); + + const phase = await phaseRepo.create({ + initiativeId: initiative.id, + name: 'Test Phase', + status: 'pending_review', + }); + + const project = await projectRepo.create({ + name: 'Test Project', + url: 'https://github.com/test/repo', + }); + + await projectRepo.addProjectToInitiative(initiative.id, project.id); + + return { + repos: { initiativeRepo, phaseRepo, projectRepo }, + data: { phaseId: phase.id, initiativeId: initiative.id, projectId: project.id }, + }; +} + +async function seedDatabaseNoProjects(): Promise<{ + repos: { + initiativeRepo: DrizzleInitiativeRepository; + phaseRepo: DrizzlePhaseRepository; + projectRepo: DrizzleProjectRepository; + }; + data: { phaseId: string }; +}> { + const db = createTestDatabase(); + const initiativeRepo = new DrizzleInitiativeRepository(db); + const phaseRepo = new DrizzlePhaseRepository(db); + const projectRepo = new DrizzleProjectRepository(db); + + const initiative = await initiativeRepo.create({ + name: 'Test Initiative No Projects', + status: 'active', + branch: 'main', + }); + + const phase = await phaseRepo.create({ + initiativeId: initiative.id, + name: 'Empty Phase', + status: 'pending_review', + }); + + return { + repos: { initiativeRepo, phaseRepo, projectRepo }, + data: { phaseId: phase.id }, + }; +} + +function makeCaller( + branchManager: BranchManager, + repos: { + initiativeRepo: DrizzleInitiativeRepository; + phaseRepo: DrizzlePhaseRepository; + projectRepo: DrizzleProjectRepository; + }, +) { + const ctx: TRPCContext = { + eventBus: createMockEventBus(), + serverStartedAt: null, + processCount: 0, + branchManager, + initiativeRepository: repos.initiativeRepo, + phaseRepository: repos.phaseRepo, + projectRepository: repos.projectRepo, + workspaceRoot: '/fake/workspace', + }; + return createCaller(ctx); +} + +// ============================================================================= +// Tests +// ============================================================================= + +beforeEach(() => { + // Clear caches between tests to ensure isolation + phaseMetaCache.invalidateByPrefix(''); + fileDiffCache.invalidateByPrefix(''); +}); + +describe('getPhaseReviewDiff caching', () => { + it('second call for same phase/HEAD returns cached result without calling git again', async () => { + const { repos, data } = await seedDatabase(); + const branchManager = makeMockBranchManager(); + const diffBranchesSpy = vi.spyOn(branchManager, 'diffBranchesStat'); + const caller = makeCaller(branchManager, repos); + + await caller.getPhaseReviewDiff({ phaseId: data.phaseId }); + await caller.getPhaseReviewDiff({ phaseId: data.phaseId }); + + expect(diffBranchesSpy).toHaveBeenCalledTimes(1); + }); + + it('after cache invalidation, next call re-runs git diff', async () => { + const { repos, data } = await seedDatabase(); + const branchManager = makeMockBranchManager(); + const diffBranchesSpy = vi.spyOn(branchManager, 'diffBranchesStat'); + const caller = makeCaller(branchManager, repos); + + await caller.getPhaseReviewDiff({ phaseId: data.phaseId }); + expect(diffBranchesSpy).toHaveBeenCalledTimes(1); + + // Simulate a task merge → cache invalidated + phaseMetaCache.invalidateByPrefix(`${data.phaseId}:`); + + await caller.getPhaseReviewDiff({ phaseId: data.phaseId }); + expect(diffBranchesSpy).toHaveBeenCalledTimes(2); + }); + + it('different HEAD hashes for same phase are treated as distinct cache entries', async () => { + const { repos, data } = await seedDatabase(); + const branchManager = makeMockBranchManager(); + const diffBranchesSpy = vi.spyOn(branchManager, 'diffBranchesStat'); + const caller = makeCaller(branchManager, repos); + + // First call with headHash = 'abc123' + vi.spyOn(branchManager, 'getHeadCommitHash').mockResolvedValueOnce('abc123'); + await caller.getPhaseReviewDiff({ phaseId: data.phaseId }); + + // Second call with headHash = 'def456' (simulates a new commit) + vi.spyOn(branchManager, 'getHeadCommitHash').mockResolvedValueOnce('def456'); + await caller.getPhaseReviewDiff({ phaseId: data.phaseId }); + + expect(diffBranchesSpy).toHaveBeenCalledTimes(2); + }); + + it('throws NOT_FOUND for nonexistent phaseId', async () => { + const { repos } = await seedDatabase(); + const caller = makeCaller(makeMockBranchManager(), repos); + + await expect(caller.getPhaseReviewDiff({ phaseId: 'nonexistent' })) + .rejects.toMatchObject({ code: 'NOT_FOUND' }); + }); + + it('phase with no projects returns empty result without calling git', async () => { + const { repos, data } = await seedDatabaseNoProjects(); + const branchManager = makeMockBranchManager(); + const diffBranchesSpy = vi.spyOn(branchManager, 'diffBranchesStat'); + const caller = makeCaller(branchManager, repos); + + const result = await caller.getPhaseReviewDiff({ phaseId: data.phaseId }); + expect(diffBranchesSpy).not.toHaveBeenCalled(); + expect(result).toHaveProperty('phaseName'); + }); +}); diff --git a/apps/server/trpc/routers/phase.ts b/apps/server/trpc/routers/phase.ts index 0d9c999..ef5e31c 100644 --- a/apps/server/trpc/routers/phase.ts +++ b/apps/server/trpc/routers/phase.ts @@ -11,6 +11,7 @@ import { requirePhaseRepository, requireTaskRepository, requireBranchManager, re import { phaseBranchName } from '../../git/branch-naming.js'; import { ensureProjectClone } from '../../git/project-clones.js'; import type { FileStatEntry } from '../../git/types.js'; +import { phaseMetaCache } from '../../review/diff-cache.js'; export function phaseProcedures(publicProcedure: ProcedureBuilder) { return { @@ -235,6 +236,26 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { const diffBase = (phase.status === 'completed' && phase.mergeBase) ? phase.mergeBase : initBranch; const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId); + + if (projects.length === 0) { + return { + phaseName: phase.name, + sourceBranch: phBranch, + targetBranch: initBranch, + files: [], + totalAdditions: 0, + totalDeletions: 0, + }; + } + + const firstClone = await ensureProjectClone(projects[0], ctx.workspaceRoot!); + const headHash = await branchManager.getHeadCommitHash(firstClone, phBranch); + const cacheKey = `${input.phaseId}:${headHash}`; + + type PhaseReviewDiffResult = { phaseName: string; sourceBranch: string; targetBranch: string; files: FileStatEntry[]; totalAdditions: number; totalDeletions: number }; + const cached = phaseMetaCache.get(cacheKey) as PhaseReviewDiffResult | undefined; + if (cached) return cached; + const files: FileStatEntry[] = []; for (const project of projects) { @@ -255,7 +276,7 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { const totalAdditions = files.reduce((sum, f) => sum + f.additions, 0); const totalDeletions = files.reduce((sum, f) => sum + f.deletions, 0); - return { + const result = { phaseName: phase.name, sourceBranch: phBranch, targetBranch: initBranch, @@ -263,6 +284,9 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { totalAdditions, totalDeletions, }; + + phaseMetaCache.set(cacheKey, result); + return result; }), getFileDiff: publicProcedure diff --git a/docs/server-api.md b/docs/server-api.md index eb5e5ef..887bccd 100644 --- a/docs/server-api.md +++ b/docs/server-api.md @@ -118,7 +118,7 @@ Each procedure uses `require*Repository(ctx)` helpers that throw `TRPCError(INTE | listInitiativePhaseDependencies | query | All dependency edges | | getPhaseDependencies | query | What this phase depends on | | getPhaseDependents | query | What depends on this phase | -| getPhaseReviewDiff | query | File-level metadata for pending_review phase: `{phaseName, sourceBranch, targetBranch, files: FileStatEntry[], totalAdditions, totalDeletions}` — no hunk content | +| getPhaseReviewDiff | query | File-level metadata for pending_review phase: `{phaseName, sourceBranch, targetBranch, files: FileStatEntry[], totalAdditions, totalDeletions}` — no hunk content. Results are cached in-memory by `phaseId:headHash` (TTL: `REVIEW_DIFF_CACHE_TTL_MS`, default 5 min). Cache is invalidated when a task merges into the phase branch. | | getFileDiff | query | Per-file unified diff on demand: `{phaseId, filePath, projectId?}` → `{binary: boolean, rawDiff: string}`; `filePath` must be URL-encoded; binary files return `{binary: true, rawDiff: ''}` | | getPhaseReviewCommits | query | List commits between initiative and phase branch | | getCommitDiff | query | Diff for a single commit (by hash) in a phase |