tas/Codewalkers
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: split getPhaseReviewDiff into metadata + add getFileDiff procedure

Browse Source 
Rewrites getPhaseReviewDiff to return file-level metadata (path, status,
additions, deletions) instead of a raw diff string, eliminating 10MB+
payloads for large repos. Adds getFileDiff for on-demand per-file hunk
content with binary detection via numstat. Multi-project initiatives
prefix file paths with the project name to avoid collisions.

Adds integration tests that use real local git repos + in-memory SQLite
to verify both procedures end-to-end (binary files, deleted files,
spaces in paths, error cases).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Lukas May
2026-03-06 19:45:57 +01:00
parent 05eb160749
commit 4890721a92
3 changed files with 331 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

256
apps/server/test/integration/phase-review-diff.test.ts Normal file
View File

@@ -0,0 +1,256 @@
/**
* Integration tests for getPhaseReviewDiff and getFileDiff tRPC procedures.
*
* Uses real git repos on disk (no cassettes) + an in-memory SQLite database.
* No network calls — purely local git operations.
*/
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
import { mkdtemp, rm, writeFile, mkdir } from 'node:fs/promises';
import { tmpdir } from 'node:os';
import path from 'node:path';
import { simpleGit } from 'simple-git';
import { router, publicProcedure, createCallerFactory } from '../../trpc/trpc.js';
import { phaseProcedures } from '../../trpc/routers/phase.js';
import { createTestDatabase } from '../../db/repositories/drizzle/test-helpers.js';
import {
DrizzleInitiativeRepository,
DrizzlePhaseRepository,
DrizzleProjectRepository,
} from '../../db/repositories/drizzle/index.js';
import { SimpleGitBranchManager } from '../../git/simple-git-branch-manager.js';
import { getProjectCloneDir } from '../../git/project-clones.js';
import type { TRPCContext } from '../../trpc/context.js';
// ============================================================================
// Test router & caller factory
// ============================================================================
const testRouter = router({
...phaseProcedures(publicProcedure),
});
const createCaller = createCallerFactory(testRouter);
// ============================================================================
// Shared test state (set up once for the whole suite)
// ============================================================================
let workspaceRoot: string;
let cleanup: () => Promise<void>;
let phaseId: string;
let pendingPhaseId: string;
/**
* Build the test git repo with the required branches and files.
*
* Repo layout on the phase branch vs main:
* file1.txt through file5.txt — added (10–20 lines each)
* photo.bin — binary file (Buffer.alloc(32))
* gone.txt — deleted (existed on main)
* has space.txt — added (contains text)
*/
async function setupGitRepo(clonePath: string): Promise<void> {
await mkdir(clonePath, { recursive: true });
const git = simpleGit(clonePath);
await git.init();
await git.addConfig('user.email', 'test@example.com');
await git.addConfig('user.name', 'Test');
// Commit gone.txt on main so it can be deleted on the phase branch
await writeFile(path.join(clonePath, 'gone.txt'), Array.from({ length: 5 }, (_, i) => `line ${i + 1}`).join('\n') + '\n');
await git.add('gone.txt');
await git.commit('Initial commit on main');
// Create phase branch from main
// phaseBranchName('main', 'test-phase') => 'main-phase-test-phase'
await git.checkoutLocalBranch('main-phase-test-phase');
// Add 5 text files
for (let i = 1; i <= 5; i++) {
const lines = Array.from({ length: 15 }, (_, j) => `Line ${j + 1} in file${i}`).join('\n') + '\n';
await writeFile(path.join(clonePath, `file${i}.txt`), lines);
}
await git.add(['file1.txt', 'file2.txt', 'file3.txt', 'file4.txt', 'file5.txt']);
// Add binary file
await writeFile(path.join(clonePath, 'photo.bin'), Buffer.alloc(32));
await git.add('photo.bin');
// Delete gone.txt
await git.rm(['gone.txt']);
// Add file with space in name
await writeFile(path.join(clonePath, 'has space.txt'), 'content with spaces\n');
await git.add('has space.txt');
await git.commit('Phase branch changes');
}
beforeAll(async () => {
// Create workspace root temp dir
workspaceRoot = await mkdtemp(path.join(tmpdir(), 'cw-phase-diff-test-'));
cleanup = async () => {
await rm(workspaceRoot, { recursive: true, force: true });
};
// Set up in-memory database
const db = createTestDatabase();
const initiativeRepo = new DrizzleInitiativeRepository(db);
const phaseRepo = new DrizzlePhaseRepository(db);
const projectRepo = new DrizzleProjectRepository(db);
// Create initiative with branch='main'
const initiative = await initiativeRepo.create({
name: 'Test Initiative',
branch: 'main',
});
// Create project — we'll set up the git repo at the expected clone path
const project = await projectRepo.create({
name: 'test-repo',
url: 'file:///dev/null', // won't be cloned — we create the repo directly
defaultBranch: 'main',
});
// Link project to initiative
await projectRepo.addProjectToInitiative(initiative.id, project.id);
// Set up git repo at the expected clone path
const relPath = getProjectCloneDir(project.name, project.id);
const clonePath = path.join(workspaceRoot, relPath);
await setupGitRepo(clonePath);
// Create reviewable phase (pending_review)
const phase = await phaseRepo.create({
initiativeId: initiative.id,
name: 'test-phase',
status: 'pending_review',
});
phaseId = phase.id;
// Create a non-reviewable phase (pending) for error test
const pendingPhase = await phaseRepo.create({
initiativeId: initiative.id,
name: 'pending-phase',
status: 'pending',
});
pendingPhaseId = pendingPhase.id;
// Store db and repos so the caller can use them
// (stored in module-level vars to be accessed in test helper)
Object.assign(sharedCtx, {
initiativeRepository: initiativeRepo,
phaseRepository: phaseRepo,
projectRepository: projectRepo,
branchManager: new SimpleGitBranchManager(),
workspaceRoot,
});
});
afterAll(async () => {
await cleanup?.();
});
// ============================================================================
// Shared context (filled in beforeAll)
// ============================================================================
const sharedCtx: Partial<TRPCContext> = {
eventBus: { emit: () => {}, on: () => {}, off: () => {}, once: () => {} } as any,
serverStartedAt: null,
processCount: 0,
};
function getCaller() {
return createCaller(sharedCtx as TRPCContext);
}
// ============================================================================
// Tests: getPhaseReviewDiff
// ============================================================================
describe('getPhaseReviewDiff', () => {
it('returns files array with correct metadata and no rawDiff field', async () => {
const start = Date.now();
const result = await getCaller().getPhaseReviewDiff({ phaseId });
const elapsed = Date.now() - start;
expect(result).not.toHaveProperty('rawDiff');
expect(result.files).toBeInstanceOf(Array);
// 5 text + 1 binary + 1 deleted + 1 spaced = 8 files
expect(result.files.length).toBeGreaterThanOrEqual(7);
expect(elapsed).toBeLessThan(3000);
});
it('includes binary file with status=binary, additions=0, deletions=0', async () => {
const result = await getCaller().getPhaseReviewDiff({ phaseId });
const bin = result.files.find((f) => f.path === 'photo.bin');
expect(bin?.status).toBe('binary');
expect(bin?.additions).toBe(0);
expect(bin?.deletions).toBe(0);
});
it('includes deleted file with status=deleted and nonzero deletions', async () => {
const result = await getCaller().getPhaseReviewDiff({ phaseId });
const del = result.files.find((f) => f.path === 'gone.txt');
expect(del?.status).toBe('deleted');
expect(del?.deletions).toBeGreaterThan(0);
});
it('computes totalAdditions and totalDeletions as sums over files', async () => {
const result = await getCaller().getPhaseReviewDiff({ phaseId });
const sumAdd = result.files.reduce((s, f) => s + f.additions, 0);
const sumDel = result.files.reduce((s, f) => s + f.deletions, 0);
expect(result.totalAdditions).toBe(sumAdd);
expect(result.totalDeletions).toBe(sumDel);
});
it('throws NOT_FOUND for unknown phaseId', async () => {
const err = await getCaller().getPhaseReviewDiff({ phaseId: 'nonexistent' }).catch((e) => e);
expect(err.code).toBe('NOT_FOUND');
});
it('throws BAD_REQUEST for phase not in reviewable status', async () => {
const err = await getCaller().getPhaseReviewDiff({ phaseId: pendingPhaseId }).catch((e) => e);
expect(err.code).toBe('BAD_REQUEST');
});
});
// ============================================================================
// Tests: getFileDiff
// ============================================================================
describe('getFileDiff', () => {
it('returns rawDiff with unified diff for a normal file, under 1 second', async () => {
const start = Date.now();
const result = await getCaller().getFileDiff({ phaseId, filePath: 'file1.txt' });
const elapsed = Date.now() - start;
expect(result.binary).toBe(false);
expect(result.rawDiff).toContain('+');
expect(elapsed).toBeLessThan(1000);
});
it('returns binary=true and rawDiff="" for binary file', async () => {
const result = await getCaller().getFileDiff({ phaseId, filePath: 'photo.bin' });
expect(result.binary).toBe(true);
expect(result.rawDiff).toBe('');
});
it('returns removal hunks for a deleted file', async () => {
const result = await getCaller().getFileDiff({ phaseId, filePath: 'gone.txt' });
expect(result.binary).toBe(false);
expect(result.rawDiff).toContain('-');
});
it('handles URL-encoded file path with space', async () => {
const result = await getCaller().getFileDiff({
phaseId,
filePath: encodeURIComponent('has space.txt'),
});
expect(result.binary).toBe(false);
expect(result.rawDiff).toContain('has space.txt');
});
});

79
apps/server/trpc/routers/phase.ts
View File

@@ -4,11 +4,13 @@
import { TRPCError } from '@trpc/server';
import { z } from 'zod';
import { simpleGit } from 'simple-git';
import type { Phase } from '../../db/schema.js';
import type { ProcedureBuilder } from '../trpc.js';
import { requirePhaseRepository, requireTaskRepository, requireBranchManager, requireInitiativeRepository, requireProjectRepository, requireExecutionOrchestrator, requireReviewCommentRepository, requireChangeSetRepository } from './_helpers.js';
import { phaseBranchName } from '../../git/branch-naming.js';
import { ensureProjectClone } from '../../git/project-clones.js';
import type { FileStatEntry } from '../../git/types.js';
export function phaseProcedures(publicProcedure: ProcedureBuilder) {
return {
@@ -230,28 +232,93 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) {
const initBranch = initiative.branch;
const phBranch = phaseBranchName(initBranch, phase.name);
// For completed phases, use stored merge base; for pending_review, use initiative branch
const diffBase = (phase.status === 'completed' && phase.mergeBase) ? phase.mergeBase : initBranch;
const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId);
let rawDiff = '';
const files: FileStatEntry[] = [];
for (const project of projects) {
const clonePath = await ensureProjectClone(project, ctx.workspaceRoot!);
const diff = await branchManager.diffBranches(clonePath, diffBase, phBranch);
if (diff) {
rawDiff += diff + '\n';
const entries = await branchManager.diffBranchesStat(clonePath, diffBase, phBranch);
for (const entry of entries) {
const tagged: FileStatEntry = { ...entry, projectId: project.id };
if (projects.length > 1) {
tagged.path = `${project.name}/${entry.path}`;
if (entry.oldPath) {
tagged.oldPath = `${project.name}/${entry.oldPath}`;
}
}
files.push(tagged);
}
}
const totalAdditions = files.reduce((sum, f) => sum + f.additions, 0);
const totalDeletions = files.reduce((sum, f) => sum + f.deletions, 0);
return {
phaseName: phase.name,
sourceBranch: phBranch,
targetBranch: initBranch,
rawDiff,
files,
totalAdditions,
totalDeletions,
};
}),
getFileDiff: publicProcedure
.input(z.object({
phaseId: z.string().min(1),
filePath: z.string().min(1),
projectId: z.string().optional(),
}))
.query(async ({ ctx, input }) => {
const phaseRepo = requirePhaseRepository(ctx);
const initiativeRepo = requireInitiativeRepository(ctx);
const projectRepo = requireProjectRepository(ctx);
const branchManager = requireBranchManager(ctx);
const phase = await phaseRepo.findById(input.phaseId);
if (!phase) {
throw new TRPCError({ code: 'NOT_FOUND', message: `Phase '${input.phaseId}' not found` });
}
if (phase.status !== 'pending_review' && phase.status !== 'completed') {
throw new TRPCError({ code: 'BAD_REQUEST', message: `Phase is not reviewable (status: ${phase.status})` });
}
const initiative = await initiativeRepo.findById(phase.initiativeId);
if (!initiative?.branch) {
throw new TRPCError({ code: 'BAD_REQUEST', message: 'Initiative has no branch configured' });
}
const initBranch = initiative.branch;
const phBranch = phaseBranchName(initBranch, phase.name);
const diffBase = (phase.status === 'completed' && phase.mergeBase) ? phase.mergeBase : initBranch;
const decodedPath = decodeURIComponent(input.filePath);
const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId);
let clonePath: string;
if (input.projectId) {
const project = projects.find((p) => p.id === input.projectId);
if (!project) {
throw new TRPCError({ code: 'NOT_FOUND', message: `Project '${input.projectId}' not found for this phase` });
}
clonePath = await ensureProjectClone(project, ctx.workspaceRoot!);
} else {
clonePath = await ensureProjectClone(projects[0], ctx.workspaceRoot!);
}
const git = simpleGit(clonePath);
// Binary files appear as "-\t-\t<path>" 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 rawDiff = await branchManager.diffFileSingle(clonePath, diffBase, phBranch, decodedPath);
return { binary: false, rawDiff };
}),
approvePhaseReview: publicProcedure
.input(z.object({ phaseId: z.string().min(1) }))
.mutation(async ({ ctx, input }) => {

3
docs/server-api.md
View File

@@ -118,7 +118,8 @@ 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 | Full branch diff for pending_review phase |
| getPhaseReviewDiff | query | File-level metadata for pending_review phase: `{phaseName, sourceBranch, targetBranch, files: FileStatEntry[], totalAdditions, totalDeletions}` — no hunk content |
| 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 |
| approvePhaseReview | mutation | Approve and merge phase branch |