mirror of
https://github.com/AutoMaker-Org/automaker.git
synced 2026-02-03 21:03:08 +00:00
refactor: standardize PR state representation across the application
Updated the PR state handling to use a consistent uppercase format ('OPEN', 'MERGED', 'CLOSED') throughout the codebase. This includes changes to the worktree metadata interface, PR creation logic, and related tests to ensure uniformity and prevent potential mismatches in state representation.
Additionally, modified the GitHub PR fetching logic to retrieve all PR states, allowing for better detection of state changes.
This refactor enhances clarity and consistency in how PR states are managed and displayed.
This commit is contained in:
@@ -9,11 +9,15 @@ import * as path from 'path';
|
|||||||
/** Maximum length for sanitized branch names in filesystem paths */
|
/** Maximum length for sanitized branch names in filesystem paths */
|
||||||
const MAX_SANITIZED_BRANCH_PATH_LENGTH = 200;
|
const MAX_SANITIZED_BRANCH_PATH_LENGTH = 200;
|
||||||
|
|
||||||
|
/** GitHub PR states as returned by the GitHub API */
|
||||||
|
export type PRState = 'OPEN' | 'MERGED' | 'CLOSED';
|
||||||
|
|
||||||
export interface WorktreePRInfo {
|
export interface WorktreePRInfo {
|
||||||
number: number;
|
number: number;
|
||||||
url: string;
|
url: string;
|
||||||
title: string;
|
title: string;
|
||||||
state: string;
|
/** PR state: OPEN, MERGED, or CLOSED */
|
||||||
|
state: PRState;
|
||||||
createdAt: string;
|
createdAt: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -268,11 +268,12 @@ export function createCreatePRHandler() {
|
|||||||
prAlreadyExisted = true;
|
prAlreadyExisted = true;
|
||||||
|
|
||||||
// Store the existing PR info in metadata
|
// Store the existing PR info in metadata
|
||||||
|
// GitHub CLI returns uppercase states: OPEN, MERGED, CLOSED
|
||||||
await updateWorktreePRInfo(effectiveProjectPath, branchName, {
|
await updateWorktreePRInfo(effectiveProjectPath, branchName, {
|
||||||
number: existingPr.number,
|
number: existingPr.number,
|
||||||
url: existingPr.url,
|
url: existingPr.url,
|
||||||
title: existingPr.title || title,
|
title: existingPr.title || title,
|
||||||
state: existingPr.state || 'open',
|
state: existingPr.state || 'OPEN',
|
||||||
createdAt: new Date().toISOString(),
|
createdAt: new Date().toISOString(),
|
||||||
});
|
});
|
||||||
logger.debug(
|
logger.debug(
|
||||||
@@ -319,11 +320,12 @@ export function createCreatePRHandler() {
|
|||||||
|
|
||||||
if (prNumber) {
|
if (prNumber) {
|
||||||
try {
|
try {
|
||||||
|
// Note: GitHub doesn't have a 'DRAFT' state - drafts still show as 'OPEN'
|
||||||
await updateWorktreePRInfo(effectiveProjectPath, branchName, {
|
await updateWorktreePRInfo(effectiveProjectPath, branchName, {
|
||||||
number: prNumber,
|
number: prNumber,
|
||||||
url: prUrl,
|
url: prUrl,
|
||||||
title,
|
title,
|
||||||
state: draft ? 'draft' : 'open',
|
state: 'OPEN',
|
||||||
createdAt: new Date().toISOString(),
|
createdAt: new Date().toISOString(),
|
||||||
});
|
});
|
||||||
logger.debug(`Stored PR info for branch ${branchName}: PR #${prNumber}`);
|
logger.debug(`Stored PR info for branch ${branchName}: PR #${prNumber}`);
|
||||||
@@ -352,11 +354,12 @@ export function createCreatePRHandler() {
|
|||||||
prNumber = existingPr.number;
|
prNumber = existingPr.number;
|
||||||
prAlreadyExisted = true;
|
prAlreadyExisted = true;
|
||||||
|
|
||||||
|
// GitHub CLI returns uppercase states: OPEN, MERGED, CLOSED
|
||||||
await updateWorktreePRInfo(effectiveProjectPath, branchName, {
|
await updateWorktreePRInfo(effectiveProjectPath, branchName, {
|
||||||
number: existingPr.number,
|
number: existingPr.number,
|
||||||
url: existingPr.url,
|
url: existingPr.url,
|
||||||
title: existingPr.title || title,
|
title: existingPr.title || title,
|
||||||
state: existingPr.state || 'open',
|
state: existingPr.state || 'OPEN',
|
||||||
createdAt: new Date().toISOString(),
|
createdAt: new Date().toISOString(),
|
||||||
});
|
});
|
||||||
logger.debug(`Fetched and stored existing PR: #${existingPr.number}`);
|
logger.debug(`Fetched and stored existing PR: #${existingPr.number}`);
|
||||||
|
|||||||
@@ -14,7 +14,12 @@ import path from 'path';
|
|||||||
import * as secureFs from '../../../lib/secure-fs.js';
|
import * as secureFs from '../../../lib/secure-fs.js';
|
||||||
import { isGitRepo } from '@automaker/git-utils';
|
import { isGitRepo } from '@automaker/git-utils';
|
||||||
import { getErrorMessage, logError, normalizePath, execEnv, isGhCliAvailable } from '../common.js';
|
import { getErrorMessage, logError, normalizePath, execEnv, isGhCliAvailable } from '../common.js';
|
||||||
import { readAllWorktreeMetadata, type WorktreePRInfo } from '../../../lib/worktree-metadata.js';
|
import {
|
||||||
|
readAllWorktreeMetadata,
|
||||||
|
updateWorktreePRInfo,
|
||||||
|
type WorktreePRInfo,
|
||||||
|
type PRState,
|
||||||
|
} from '../../../lib/worktree-metadata.js';
|
||||||
import { createLogger } from '@automaker/utils';
|
import { createLogger } from '@automaker/utils';
|
||||||
import {
|
import {
|
||||||
checkGitHubRemote,
|
checkGitHubRemote,
|
||||||
@@ -168,8 +173,11 @@ async function getGitHubRemoteStatus(projectPath: string): Promise<GitHubRemoteS
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Fetch open PRs from GitHub and create a map of branch name to PR info.
|
* Fetch all PRs from GitHub and create a map of branch name to PR info.
|
||||||
* This allows detecting PRs that were created outside the app.
|
* Uses --state all to include merged/closed PRs, allowing detection of
|
||||||
|
* state changes (e.g., when a PR is merged on GitHub).
|
||||||
|
*
|
||||||
|
* This also allows detecting PRs that were created outside the app.
|
||||||
*
|
*
|
||||||
* Uses cached GitHub remote status to avoid repeated warnings when the
|
* Uses cached GitHub remote status to avoid repeated warnings when the
|
||||||
* project doesn't have a GitHub remote configured.
|
* project doesn't have a GitHub remote configured.
|
||||||
@@ -192,9 +200,9 @@ async function fetchGitHubPRs(projectPath: string): Promise<Map<string, Worktree
|
|||||||
? `-R ${remoteStatus.owner}/${remoteStatus.repo}`
|
? `-R ${remoteStatus.owner}/${remoteStatus.repo}`
|
||||||
: '';
|
: '';
|
||||||
|
|
||||||
// Fetch open PRs from GitHub
|
// Fetch all PRs from GitHub (including merged/closed to detect state changes)
|
||||||
const { stdout } = await execAsync(
|
const { stdout } = await execAsync(
|
||||||
`gh pr list ${repoFlag} --state open --json number,title,url,state,headRefName,createdAt --limit 1000`,
|
`gh pr list ${repoFlag} --state all --json number,title,url,state,headRefName,createdAt --limit 1000`,
|
||||||
{ cwd: projectPath, env: execEnv, timeout: 15000 }
|
{ cwd: projectPath, env: execEnv, timeout: 15000 }
|
||||||
);
|
);
|
||||||
|
|
||||||
@@ -212,7 +220,8 @@ async function fetchGitHubPRs(projectPath: string): Promise<Map<string, Worktree
|
|||||||
number: pr.number,
|
number: pr.number,
|
||||||
url: pr.url,
|
url: pr.url,
|
||||||
title: pr.title,
|
title: pr.title,
|
||||||
state: pr.state,
|
// GitHub CLI returns state as uppercase: OPEN, MERGED, CLOSED
|
||||||
|
state: pr.state as PRState,
|
||||||
createdAt: pr.createdAt,
|
createdAt: pr.createdAt,
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
@@ -351,23 +360,36 @@ export function createListHandler() {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Add PR info from metadata or GitHub for each worktree
|
// Assign PR info to each worktree, preferring fresh GitHub data over cached metadata.
|
||||||
// Only fetch GitHub PRs if includeDetails is requested (performance optimization)
|
// Only fetch GitHub PRs if includeDetails is requested (performance optimization).
|
||||||
|
// Uses --state all to detect merged/closed PRs, limited to 1000 recent PRs.
|
||||||
const githubPRs = includeDetails
|
const githubPRs = includeDetails
|
||||||
? await fetchGitHubPRs(projectPath)
|
? await fetchGitHubPRs(projectPath)
|
||||||
: new Map<string, WorktreePRInfo>();
|
: new Map<string, WorktreePRInfo>();
|
||||||
|
|
||||||
for (const worktree of worktrees) {
|
for (const worktree of worktrees) {
|
||||||
const metadata = allMetadata.get(worktree.branch);
|
const metadata = allMetadata.get(worktree.branch);
|
||||||
if (metadata?.pr) {
|
const githubPR = githubPRs.get(worktree.branch);
|
||||||
// Use stored metadata (more complete info)
|
|
||||||
worktree.pr = metadata.pr;
|
if (githubPR) {
|
||||||
} else if (includeDetails) {
|
// Prefer fresh GitHub data (it has the current state)
|
||||||
// Fall back to GitHub PR detection only when includeDetails is requested
|
worktree.pr = githubPR;
|
||||||
const githubPR = githubPRs.get(worktree.branch);
|
|
||||||
if (githubPR) {
|
// Sync metadata with GitHub state when:
|
||||||
worktree.pr = githubPR;
|
// 1. No metadata exists for this PR (PR created externally)
|
||||||
|
// 2. State has changed (e.g., merged/closed on GitHub)
|
||||||
|
const needsSync = !metadata?.pr || metadata.pr.state !== githubPR.state;
|
||||||
|
if (needsSync) {
|
||||||
|
// Fire and forget - don't block the response
|
||||||
|
updateWorktreePRInfo(projectPath, worktree.branch, githubPR).catch((err) => {
|
||||||
|
logger.warn(
|
||||||
|
`Failed to update PR info for ${worktree.branch}: ${getErrorMessage(err)}`
|
||||||
|
);
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
} else if (metadata?.pr) {
|
||||||
|
// Fall back to stored metadata (for PRs not in recent GitHub response)
|
||||||
|
worktree.pr = metadata.pr;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -121,7 +121,7 @@ describe('worktree-metadata.ts', () => {
|
|||||||
number: 123,
|
number: 123,
|
||||||
url: 'https://github.com/owner/repo/pull/123',
|
url: 'https://github.com/owner/repo/pull/123',
|
||||||
title: 'Test PR',
|
title: 'Test PR',
|
||||||
state: 'open',
|
state: 'OPEN',
|
||||||
createdAt: new Date().toISOString(),
|
createdAt: new Date().toISOString(),
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
@@ -158,7 +158,7 @@ describe('worktree-metadata.ts', () => {
|
|||||||
number: 456,
|
number: 456,
|
||||||
url: 'https://github.com/owner/repo/pull/456',
|
url: 'https://github.com/owner/repo/pull/456',
|
||||||
title: 'Updated PR',
|
title: 'Updated PR',
|
||||||
state: 'closed',
|
state: 'CLOSED',
|
||||||
createdAt: new Date().toISOString(),
|
createdAt: new Date().toISOString(),
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
@@ -177,7 +177,7 @@ describe('worktree-metadata.ts', () => {
|
|||||||
number: 789,
|
number: 789,
|
||||||
url: 'https://github.com/owner/repo/pull/789',
|
url: 'https://github.com/owner/repo/pull/789',
|
||||||
title: 'New PR',
|
title: 'New PR',
|
||||||
state: 'open',
|
state: 'OPEN',
|
||||||
createdAt: new Date().toISOString(),
|
createdAt: new Date().toISOString(),
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -201,7 +201,7 @@ describe('worktree-metadata.ts', () => {
|
|||||||
number: 999,
|
number: 999,
|
||||||
url: 'https://github.com/owner/repo/pull/999',
|
url: 'https://github.com/owner/repo/pull/999',
|
||||||
title: 'Updated PR',
|
title: 'Updated PR',
|
||||||
state: 'merged',
|
state: 'MERGED',
|
||||||
createdAt: new Date().toISOString(),
|
createdAt: new Date().toISOString(),
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -224,7 +224,7 @@ describe('worktree-metadata.ts', () => {
|
|||||||
number: 111,
|
number: 111,
|
||||||
url: 'https://github.com/owner/repo/pull/111',
|
url: 'https://github.com/owner/repo/pull/111',
|
||||||
title: 'PR',
|
title: 'PR',
|
||||||
state: 'open',
|
state: 'OPEN',
|
||||||
createdAt: new Date().toISOString(),
|
createdAt: new Date().toISOString(),
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -259,7 +259,7 @@ describe('worktree-metadata.ts', () => {
|
|||||||
number: 222,
|
number: 222,
|
||||||
url: 'https://github.com/owner/repo/pull/222',
|
url: 'https://github.com/owner/repo/pull/222',
|
||||||
title: 'Has PR',
|
title: 'Has PR',
|
||||||
state: 'open',
|
state: 'OPEN',
|
||||||
createdAt: new Date().toISOString(),
|
createdAt: new Date().toISOString(),
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -297,7 +297,7 @@ describe('worktree-metadata.ts', () => {
|
|||||||
number: 333,
|
number: 333,
|
||||||
url: 'https://github.com/owner/repo/pull/333',
|
url: 'https://github.com/owner/repo/pull/333',
|
||||||
title: 'PR 3',
|
title: 'PR 3',
|
||||||
state: 'open',
|
state: 'OPEN',
|
||||||
createdAt: new Date().toISOString(),
|
createdAt: new Date().toISOString(),
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -1,8 +1,12 @@
|
|||||||
|
/** GitHub PR states as returned by the GitHub API */
|
||||||
|
export type PRState = 'OPEN' | 'MERGED' | 'CLOSED';
|
||||||
|
|
||||||
export interface WorktreePRInfo {
|
export interface WorktreePRInfo {
|
||||||
number: number;
|
number: number;
|
||||||
url: string;
|
url: string;
|
||||||
title: string;
|
title: string;
|
||||||
state: string;
|
/** PR state: OPEN, MERGED, or CLOSED */
|
||||||
|
state: PRState;
|
||||||
createdAt: string;
|
createdAt: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -43,7 +47,8 @@ export interface PRInfo {
|
|||||||
number: number;
|
number: number;
|
||||||
title: string;
|
title: string;
|
||||||
url: string;
|
url: string;
|
||||||
state: string;
|
/** PR state: OPEN, MERGED, or CLOSED */
|
||||||
|
state: PRState;
|
||||||
author: string;
|
author: string;
|
||||||
body: string;
|
body: string;
|
||||||
comments: Array<{
|
comments: Array<{
|
||||||
|
|||||||
Reference in New Issue
Block a user