fix: use per-review comment fetching for Forgejo compatibility
Forgejo doesn't have the flat GET /pulls/{number}/comments endpoint
(returns 404). Replace pulls.listReviewComments() with a 2-phase
approach: fetch reviews first, then pulls.listCommentsForReview()
per-review in parallel. Updates tests to mock the new per-review API.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
1 parent
efe30422e7
commit
b4a6c34853
2 files changed
+48
-25
No files matched your search
+25
-11
@@ -210,14 +210,16 @@ export async function fetchForgejoData({
|
|||||||
let rawReviews: GitHubReview[] = [];
|
let rawReviews: GitHubReview[] = [];
|
||||||
|
|
||||||
if (isPR) {
|
if (isPR) {
|
||||||
// Fetch PR details, files, commits, comments, reviews, and inline comments in parallel
|
// Phase 1: Fetch PR details, files, commits, comments, and reviews in parallel.
|
||||||
|
// Note: Forgejo doesn't have a flat /pulls/{number}/comments endpoint for
|
||||||
|
// review comments (returns 404). Instead, fetch review comments per-review
|
||||||
|
// in phase 2 after we have the review IDs.
|
||||||
const [
|
const [
|
||||||
prResponse,
|
prResponse,
|
||||||
filesResponse,
|
filesResponse,
|
||||||
commitsResponse,
|
commitsResponse,
|
||||||
commentsResponse,
|
commentsResponse,
|
||||||
reviewsResponse,
|
reviewsResponse,
|
||||||
reviewCommentsResponse,
|
|
||||||
] = await Promise.all([
|
] = await Promise.all([
|
||||||
octokits.rest.pulls.get({ owner, repo, pull_number: number }),
|
octokits.rest.pulls.get({ owner, repo, pull_number: number }),
|
||||||
octokits.rest.pulls.listFiles({
|
octokits.rest.pulls.listFiles({
|
||||||
@@ -244,23 +246,35 @@ export async function fetchForgejoData({
|
|||||||
pull_number: number,
|
pull_number: number,
|
||||||
per_page: 100,
|
per_page: 100,
|
||||||
}),
|
}),
|
||||||
octokits.rest.pulls.listReviewComments({
|
|
||||||
owner,
|
|
||||||
repo,
|
|
||||||
pull_number: number,
|
|
||||||
per_page: 100,
|
|
||||||
}),
|
|
||||||
]);
|
]);
|
||||||
|
|
||||||
const pr = prResponse.data;
|
const pr = prResponse.data;
|
||||||
const files = filesResponse.data;
|
const files = filesResponse.data;
|
||||||
const commits = commitsResponse.data;
|
const commits = commitsResponse.data;
|
||||||
|
|
||||||
|
// Phase 2: Fetch inline comments for each review in parallel.
|
||||||
|
// Forgejo uses GET /repos/{owner}/{repo}/pulls/{index}/reviews/{id}/comments
|
||||||
|
const reviewCommentArrays = await Promise.all(
|
||||||
|
reviewsResponse.data.map(async (review) => {
|
||||||
|
try {
|
||||||
|
const { data } = await octokits.rest.pulls.listCommentsForReview({
|
||||||
|
owner,
|
||||||
|
repo,
|
||||||
|
pull_number: number,
|
||||||
|
review_id: review.id,
|
||||||
|
});
|
||||||
|
return data;
|
||||||
|
} catch {
|
||||||
|
// Some reviews (e.g. pending) may not have accessible comments
|
||||||
|
return [];
|
||||||
|
}
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
const allReviewCommentData = reviewCommentArrays.flat();
|
||||||
|
|
||||||
// Map REST responses to internal types
|
// Map REST responses to internal types
|
||||||
rawComments = mapRestComments(commentsResponse.data);
|
rawComments = mapRestComments(commentsResponse.data);
|
||||||
const mappedInlineComments = mapRestReviewComments(
|
const mappedInlineComments = mapRestReviewComments(allReviewCommentData);
|
||||||
reviewCommentsResponse.data,
|
|
||||||
);
|
|
||||||
rawReviews = mapRestReviews(reviewsResponse.data, mappedInlineComments);
|
rawReviews = mapRestReviews(reviewsResponse.data, mappedInlineComments);
|
||||||
|
|
||||||
// Map REST file objects to GitHubFile type
|
// Map REST file objects to GitHubFile type
|
||||||
|
|||||||
@@ -29,11 +29,25 @@ function createMockOctokits({
|
|||||||
filesData?: Record<string, unknown>[];
|
filesData?: Record<string, unknown>[];
|
||||||
commitsData?: Record<string, unknown>[];
|
commitsData?: Record<string, unknown>[];
|
||||||
commentsData?: Record<string, unknown>[];
|
commentsData?: Record<string, unknown>[];
|
||||||
reviewsData?: Record<string, unknown>[];
|
reviewsData?: (Record<string, unknown> & { id?: number })[];
|
||||||
reviewCommentsData?: Record<string, unknown>[];
|
reviewCommentsData?: (Record<string, unknown> & {
|
||||||
|
pull_request_review_id?: number | null;
|
||||||
|
})[];
|
||||||
issueData?: Record<string, unknown>;
|
issueData?: Record<string, unknown>;
|
||||||
userData?: Record<string, unknown>;
|
userData?: Record<string, unknown>;
|
||||||
} = {}) {
|
} = {}) {
|
||||||
|
// Build per-review mock: listCommentsForReview returns comments matching
|
||||||
|
// the requested review_id (Forgejo doesn't have a flat review-comments endpoint).
|
||||||
|
const allReviewComments = reviewCommentsData ?? [];
|
||||||
|
const listCommentsForReviewMock = jest
|
||||||
|
.fn()
|
||||||
|
.mockImplementation((params: { review_id: number }) => {
|
||||||
|
const matching = allReviewComments.filter(
|
||||||
|
(c) => c.pull_request_review_id === params.review_id,
|
||||||
|
);
|
||||||
|
return Promise.resolve({ data: matching });
|
||||||
|
});
|
||||||
|
|
||||||
return {
|
return {
|
||||||
rest: {
|
rest: {
|
||||||
pulls: {
|
pulls: {
|
||||||
@@ -41,9 +55,7 @@ function createMockOctokits({
|
|||||||
listFiles: jest.fn().mockResolvedValue({ data: filesData ?? [] }),
|
listFiles: jest.fn().mockResolvedValue({ data: filesData ?? [] }),
|
||||||
listCommits: jest.fn().mockResolvedValue({ data: commitsData ?? [] }),
|
listCommits: jest.fn().mockResolvedValue({ data: commitsData ?? [] }),
|
||||||
listReviews: jest.fn().mockResolvedValue({ data: reviewsData ?? [] }),
|
listReviews: jest.fn().mockResolvedValue({ data: reviewsData ?? [] }),
|
||||||
listReviewComments: jest
|
listCommentsForReview: listCommentsForReviewMock,
|
||||||
.fn()
|
|
||||||
.mockResolvedValue({ data: reviewCommentsData ?? [] }),
|
|
||||||
},
|
},
|
||||||
issues: {
|
issues: {
|
||||||
get: jest.fn().mockResolvedValue({ data: issueData ?? {} }),
|
get: jest.fn().mockResolvedValue({ data: issueData ?? {} }),
|
||||||
@@ -114,7 +126,7 @@ const mockIssueRestResponse = {
|
|||||||
|
|
||||||
describe("fetchForgejoData", () => {
|
describe("fetchForgejoData", () => {
|
||||||
describe("PR data fetching", () => {
|
describe("PR data fetching", () => {
|
||||||
it("should fetch PR details, files, commits, comments, reviews, and inline comments in parallel", async () => {
|
it("should fetch PR details, files, commits, comments, and reviews in parallel then per-review comments", async () => {
|
||||||
const mockOctokits = createMockOctokits({
|
const mockOctokits = createMockOctokits({
|
||||||
prData: mockPrRestResponse,
|
prData: mockPrRestResponse,
|
||||||
filesData: mockFilesRestResponse,
|
filesData: mockFilesRestResponse,
|
||||||
@@ -128,7 +140,7 @@ describe("fetchForgejoData", () => {
|
|||||||
isPR: true,
|
isPR: true,
|
||||||
});
|
});
|
||||||
|
|
||||||
// Verify all six REST calls were made
|
// Verify phase 1 REST calls (parallel)
|
||||||
expect(mockOctokits.rest.pulls.get).toHaveBeenCalledWith({
|
expect(mockOctokits.rest.pulls.get).toHaveBeenCalledWith({
|
||||||
owner: "test-owner",
|
owner: "test-owner",
|
||||||
repo: "test-repo",
|
repo: "test-repo",
|
||||||
@@ -158,12 +170,7 @@ describe("fetchForgejoData", () => {
|
|||||||
pull_number: 42,
|
pull_number: 42,
|
||||||
per_page: 100,
|
per_page: 100,
|
||||||
});
|
});
|
||||||
expect(mockOctokits.rest.pulls.listReviewComments).toHaveBeenCalledWith({
|
// Phase 2: per-review comment fetching (no reviews in this test → no calls)
|
||||||
owner: "test-owner",
|
|
||||||
repo: "test-repo",
|
|
||||||
pull_number: 42,
|
|
||||||
per_page: 100,
|
|
||||||
});
|
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should map PR fields to GitHubPullRequest type", async () => {
|
it("should map PR fields to GitHubPullRequest type", async () => {
|
||||||
@@ -412,7 +419,9 @@ describe("fetchForgejoData", () => {
|
|||||||
expect(mockOctokits.rest.pulls.listFiles).not.toHaveBeenCalled();
|
expect(mockOctokits.rest.pulls.listFiles).not.toHaveBeenCalled();
|
||||||
expect(mockOctokits.rest.pulls.listCommits).not.toHaveBeenCalled();
|
expect(mockOctokits.rest.pulls.listCommits).not.toHaveBeenCalled();
|
||||||
expect(mockOctokits.rest.pulls.listReviews).not.toHaveBeenCalled();
|
expect(mockOctokits.rest.pulls.listReviews).not.toHaveBeenCalled();
|
||||||
expect(mockOctokits.rest.pulls.listReviewComments).not.toHaveBeenCalled();
|
expect(
|
||||||
|
mockOctokits.rest.pulls.listCommentsForReview,
|
||||||
|
).not.toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should map issue fields to GitHubIssue type", async () => {
|
it("should map issue fields to GitHubIssue type", async () => {
|
||||||
|
|||||||
Reference in new issue
Block a user