fix: Show completed phase diffs in review tab

Completed phases showed "No phases pending review" because: 1. Frontend filtered only pending_review phases 2. Server rejected non-pending_review phases 3. After merge, three-dot diff returned empty (merge base moved) Fix: store pre-merge merge base hash on phase, use it to reconstruct diffs for completed phases. Frontend now shows both pending_review and completed phases with read-only mode (Merged badge) for completed ones.
2026-03-05 22:05:28 +01:00
parent cdb3de353d
commit 84250955d1
11 changed files with 158 additions and 103 deletions
--- a/apps/server/agent/file-io.test.ts
+++ b/apps/server/agent/file-io.test.ts
@@ -68,6 +68,7 @@ describe('writeInputFiles', () => {
      name: 'Phase One',
      content: 'First phase',
      status: 'pending',
+      mergeBase: null,
      createdAt: new Date(),
      updatedAt: new Date(),
    } as Phase;
--- a/apps/server/db/schema.ts
+++ b/apps/server/db/schema.ts
@@ -55,6 +55,7 @@ export const phases = sqliteTable('phases', {
  status: text('status', { enum: ['pending', 'approved', 'in_progress', 'completed', 'blocked', 'pending_review'] })
    .notNull()
    .default('pending'),
+  mergeBase: text('merge_base'),
  createdAt: integer('created_at', { mode: 'timestamp' }).notNull(),
  updatedAt: integer('updated_at', { mode: 'timestamp' }).notNull(),
 });
--- a/apps/server/drizzle/0031_add_phase_merge_base.sql
+++ b/apps/server/drizzle/0031_add_phase_merge_base.sql
@@ -0,0 +1 @@
+ALTER TABLE phases ADD COLUMN merge_base TEXT;
--- a/apps/server/drizzle/meta/_journal.json
+++ b/apps/server/drizzle/meta/_journal.json
@@ -218,6 +218,13 @@
      "when": 1772150400000,
      "tag": "0030_remove_task_approval",
      "breakpoints": true
+    },
+    {
+      "idx": 31,
+      "version": "6",
+      "when": 1772236800000,
+      "tag": "0031_add_phase_merge_base",
+      "breakpoints": true
    }
  ]
 }
--- a/apps/server/execution/orchestrator.ts
+++ b/apps/server/execution/orchestrator.ts
@@ -278,6 +278,18 @@ export class ExecutionOrchestrator {

    const projects = await this.projectRepository.findProjectsByInitiativeId(phase.initiativeId);

+    // Store merge base before merging so we can reconstruct diffs for completed phases
+    for (const project of projects) {
+      const clonePath = await ensureProjectClone(project, this.workspaceRoot);
+      try {
+        const mergeBase = await this.branchManager.getMergeBase(clonePath, initBranch, phBranch);
+        await this.phaseRepository.update(phaseId, { mergeBase });
+        break; // Only need one merge base (first project)
+      } catch {
+        // Phase branch may not exist in this project clone
+      }
+    }
+
    for (const project of projects) {
      const clonePath = await ensureProjectClone(project, this.workspaceRoot);
      const result = await this.branchManager.mergeBranch(clonePath, phBranch, initBranch);
--- a/apps/server/git/branch-manager.ts
+++ b/apps/server/git/branch-manager.ts
@@ -57,6 +57,12 @@ export interface BranchManager {
   */
  diffCommit(repoPath: string, commitHash: string): Promise<string>;

+  /**
+   * Get the merge base (common ancestor) of two branches.
+   * Returns the commit hash of the merge base.
+   */
+  getMergeBase(repoPath: string, branch1: string, branch2: string): Promise<string>;
+
  /**
   * Push a branch to a remote.
   * Defaults to 'origin' if no remote specified.
--- a/apps/server/git/simple-git-branch-manager.ts
+++ b/apps/server/git/simple-git-branch-manager.ts
@@ -141,6 +141,12 @@ export class SimpleGitBranchManager implements BranchManager {
    return git.diff([`${commitHash}~1`, commitHash]);
  }

+  async getMergeBase(repoPath: string, branch1: string, branch2: string): Promise<string> {
+    const git = simpleGit(repoPath);
+    const result = await git.raw(['merge-base', branch1, branch2]);
+    return result.trim();
+  }
+
  async pushBranch(repoPath: string, branch: string, remote = 'origin'): Promise<void> {
    const git = simpleGit(repoPath);
    await git.push(remote, branch);
--- a/apps/server/trpc/routers/phase.ts
+++ b/apps/server/trpc/routers/phase.ts
@@ -219,8 +219,8 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) {
        if (!phase) {
          throw new TRPCError({ code: 'NOT_FOUND', message: `Phase '${input.phaseId}' not found` });
        }
-        if (phase.status !== 'pending_review') {
-          throw new TRPCError({ code: 'BAD_REQUEST', message: `Phase is not pending review (status: ${phase.status})` });
+        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);
@@ -230,13 +230,15 @@ 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 = '';

        for (const project of projects) {
          const clonePath = await ensureProjectClone(project, ctx.workspaceRoot!);
-          const diff = await branchManager.diffBranches(clonePath, initBranch, phBranch);
+          const diff = await branchManager.diffBranches(clonePath, diffBase, phBranch);
          if (diff) {
            rawDiff += diff + '\n';
          }
@@ -270,8 +272,8 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) {
        if (!phase) {
          throw new TRPCError({ code: 'NOT_FOUND', message: `Phase '${input.phaseId}' not found` });
        }
-        if (phase.status !== 'pending_review') {
-          throw new TRPCError({ code: 'BAD_REQUEST', message: `Phase is not pending review (status: ${phase.status})` });
+        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);
@@ -281,13 +283,14 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) {

        const initBranch = initiative.branch;
        const phBranch = phaseBranchName(initBranch, phase.name);
+        const diffBase = (phase.status === 'completed' && phase.mergeBase) ? phase.mergeBase : initBranch;
        const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId);

        const allCommits: Array<{ hash: string; shortHash: string; message: string; author: string; date: string; filesChanged: number; insertions: number; deletions: number }> = [];

        for (const project of projects) {
          const clonePath = await ensureProjectClone(project, ctx.workspaceRoot!);
-          const commits = await branchManager.listCommits(clonePath, initBranch, phBranch);
+          const commits = await branchManager.listCommits(clonePath, diffBase, phBranch);
          allCommits.push(...commits);
        }

--- a/apps/web/src/components/review/ReviewHeader.tsx
+++ b/apps/web/src/components/review/ReviewHeader.tsx
@@ -21,11 +21,13 @@ import type { FileDiff, ReviewStatus } from "./types";
 interface PhaseOption {
  id: string;
  name: string;
+  status: string;
 }

 interface ReviewHeaderProps {
  phases: PhaseOption[];
  activePhaseId: string | null;
+  isReadOnly?: boolean;
  onPhaseSelect: (id: string) => void;
  phaseName: string;
  sourceBranch: string;
@@ -44,6 +46,7 @@ interface ReviewHeaderProps {
 export function ReviewHeader({
  phases,
  activePhaseId,
+  isReadOnly,
  onPhaseSelect,
  phaseName,
  sourceBranch,
@@ -92,6 +95,12 @@ export function ReviewHeader({
          <div className="flex gap-1 overflow-x-auto">
            {phases.map((phase) => {
              const isActive = phase.id === activePhaseId;
+              const isCompleted = phase.status === "completed";
+              const dotColor = isActive
+                ? "bg-primary"
+                : isCompleted
+                  ? "bg-status-success-dot"
+                  : "bg-status-warning-dot";
              return (
                <button
                  key={phase.id}
@@ -106,9 +115,7 @@ export function ReviewHeader({
                  `}
                >
                  <span
-                    className={`h-1.5 w-1.5 rounded-full shrink-0 ${
-                      isActive ? "bg-primary" : "bg-status-warning-dot"
-                    }`}
+                    className={`h-1.5 w-1.5 rounded-full shrink-0 ${dotColor}`}
                  />
                  {phase.name}
                </button>
@@ -171,102 +178,111 @@ export function ReviewHeader({
          {preview && <PreviewControls preview={preview} />}

          {/* Review status / actions */}
-          {status === "pending" && (
-            <>
-              <Button
-                variant="outline"
-                size="sm"
-                onClick={onRequestChanges}
-                disabled={isRequestingChanges}
-                className="h-8 text-xs px-3 border-status-error-border/50 text-status-error-fg hover:bg-status-error-bg/50 hover:border-status-error-border"
-              >
-                {isRequestingChanges ? (
-                  <Loader2 className="h-3 w-3 animate-spin" />
-                ) : (
-                  <X className="h-3 w-3" />
-                )}
-                Request Changes
-              </Button>
-              <div className="relative" ref={confirmRef}>
-                <Button
-                  size="sm"
-                  onClick={() => {
-                    if (unresolvedCount > 0) return;
-                    setShowConfirmation(true);
-                  }}
-                  disabled={unresolvedCount > 0}
-                  className="h-9 px-5 text-sm font-semibold shadow-sm"
-                >
-                  {unresolvedCount > 0 ? (
-                    <>
-                      <AlertCircle className="h-3.5 w-3.5" />
-                      {unresolvedCount} unresolved
-                    </>
-                  ) : (
-                    <>
-                      <GitMerge className="h-3.5 w-3.5" />
-                      Approve & Merge
-                    </>
-                  )}
-                </Button>
-
-                {/* Merge confirmation dropdown */}
-                {showConfirmation && (
-                  <div className="absolute right-0 top-full mt-1 z-30 w-64 rounded-lg border border-border bg-card shadow-lg p-4">
-                    <p className="text-sm font-semibold mb-3">
-                      Ready to merge?
-                    </p>
-                    <div className="space-y-1.5 mb-4">
-                      <div className="flex items-center gap-2 text-xs">
-                        <Check className="h-3.5 w-3.5 text-status-success-fg" />
-                        <span className="text-muted-foreground">
-                          0 unresolved comments
-                        </span>
-                      </div>
-                      <div className="flex items-center gap-2 text-xs">
-                        <Eye className="h-3.5 w-3.5 text-muted-foreground" />
-                        <span className="text-muted-foreground">
-                          {viewed}/{total} files viewed
-                        </span>
-                      </div>
-                    </div>
-                    <div className="flex items-center justify-end gap-2">
-                      <Button
-                        variant="ghost"
-                        size="sm"
-                        onClick={() => setShowConfirmation(false)}
-                        className="h-8 text-xs"
-                      >
-                        Cancel
-                      </Button>
-                      <Button
-                        size="sm"
-                        onClick={() => {
-                          setShowConfirmation(false);
-                          onApprove();
-                        }}
-                        className="h-8 px-4 text-xs font-semibold shadow-sm"
-                      >
-                        <GitMerge className="h-3.5 w-3.5" />
-                        Merge Now
-                      </Button>
-                    </div>
-                  </div>
-                )}
-              </div>
-            </>
-          )}
-          {status === "approved" && (
+          {isReadOnly ? (
            <Badge variant="success" size="xs">
              <Check className="h-3 w-3" />
-              Approved
-            </Badge>
-          )}
-          {status === "changes_requested" && (
-            <Badge variant="warning" size="xs">
-              <X className="h-3 w-3" />
-              Changes Requested
+              Merged
            </Badge>
+          ) : (
+            <>
+              {status === "pending" && (
+                <>
+                  <Button
+                    variant="outline"
+                    size="sm"
+                    onClick={onRequestChanges}
+                    disabled={isRequestingChanges}
+                    className="h-8 text-xs px-3 border-status-error-border/50 text-status-error-fg hover:bg-status-error-bg/50 hover:border-status-error-border"
+                  >
+                    {isRequestingChanges ? (
+                      <Loader2 className="h-3 w-3 animate-spin" />
+                    ) : (
+                      <X className="h-3 w-3" />
+                    )}
+                    Request Changes
+                  </Button>
+                  <div className="relative" ref={confirmRef}>
+                    <Button
+                      size="sm"
+                      onClick={() => {
+                        if (unresolvedCount > 0) return;
+                        setShowConfirmation(true);
+                      }}
+                      disabled={unresolvedCount > 0}
+                      className="h-9 px-5 text-sm font-semibold shadow-sm"
+                    >
+                      {unresolvedCount > 0 ? (
+                        <>
+                          <AlertCircle className="h-3.5 w-3.5" />
+                          {unresolvedCount} unresolved
+                        </>
+                      ) : (
+                        <>
+                          <GitMerge className="h-3.5 w-3.5" />
+                          Approve & Merge
+                        </>
+                      )}
+                    </Button>
+
+                    {/* Merge confirmation dropdown */}
+                    {showConfirmation && (
+                      <div className="absolute right-0 top-full mt-1 z-30 w-64 rounded-lg border border-border bg-card shadow-lg p-4">
+                        <p className="text-sm font-semibold mb-3">
+                          Ready to merge?
+                        </p>
+                        <div className="space-y-1.5 mb-4">
+                          <div className="flex items-center gap-2 text-xs">
+                            <Check className="h-3.5 w-3.5 text-status-success-fg" />
+                            <span className="text-muted-foreground">
+                              0 unresolved comments
+                            </span>
+                          </div>
+                          <div className="flex items-center gap-2 text-xs">
+                            <Eye className="h-3.5 w-3.5 text-muted-foreground" />
+                            <span className="text-muted-foreground">
+                              {viewed}/{total} files viewed
+                            </span>
+                          </div>
+                        </div>
+                        <div className="flex items-center justify-end gap-2">
+                          <Button
+                            variant="ghost"
+                            size="sm"
+                            onClick={() => setShowConfirmation(false)}
+                            className="h-8 text-xs"
+                          >
+                            Cancel
+                          </Button>
+                          <Button
+                            size="sm"
+                            onClick={() => {
+                              setShowConfirmation(false);
+                              onApprove();
+                            }}
+                            className="h-8 px-4 text-xs font-semibold shadow-sm"
+                          >
+                            <GitMerge className="h-3.5 w-3.5" />
+                            Merge Now
+                          </Button>
+                        </div>
+                      </div>
+                    )}
+                  </div>
+                </>
+              )}
+              {status === "approved" && (
+                <Badge variant="success" size="xs">
+                  <Check className="h-3 w-3" />
+                  Approved
+                </Badge>
+              )}
+              {status === "changes_requested" && (
+                <Badge variant="warning" size="xs">
+                  <X className="h-3 w-3" />
+                  Changes Requested
+                </Badge>
+              )}
+            </>
          )}
        </div>
      </div>
--- a/docs/database.md
+++ b/docs/database.md
@@ -29,7 +29,8 @@ All adapters use nanoid() for IDs, auto-manage timestamps, and use Drizzle's `.r
 | initiativeId | text FK → initiatives (cascade) | |
 | name | text NOT NULL | |
 | content | text nullable | Tiptap JSON |
-| status | text enum | 'pending' \| 'approved' \| 'in_progress' \| 'completed' \| 'blocked' |
+| status | text enum | 'pending' \| 'approved' \| 'in_progress' \| 'completed' \| 'blocked' \| 'pending_review' |
+| mergeBase | text nullable | git merge-base hash stored before phase merge, enables diff reconstruction for completed phases |
 | createdAt, updatedAt | integer/timestamp | |

 ### phase_dependencies
--- a/docs/git-process-logging.md
+++ b/docs/git-process-logging.md
@@ -45,6 +45,7 @@ Worktrees stored in `.cw-worktrees/` subdirectory of the repo. Each agent gets a
 | `remoteBranchExists(repoPath, branch)` | Check remote tracking branches (`origin/<branch>`) |
 | `listCommits(repoPath, base, head)` | List commits head has that base doesn't (with stats) |
 | `diffCommit(repoPath, commitHash)` | Get unified diff for a single commit |
+| `getMergeBase(repoPath, branch1, branch2)` | Get common ancestor commit hash |

 `remoteBranchExists` is used by `registerProject` and `updateProject` to validate that a project's default branch actually exists in the cloned repository before saving.
				`@@ -0,0 +1 @@`
				`ALTER TABLE phases ADD COLUMN merge_base TEXT;`