Skip to content

Commit

Permalink
Add fine-grained PR status tracking
Browse files Browse the repository at this point in the history
* Add new PR status values: no_pr, pr_open, pr_merged, pr_closed
* Update PR processing logic to check PR state and merged status
* Add tests for different PR states
* Update CSS with status-specific colors
  • Loading branch information
openhands-agent committed Dec 10, 2024
1 parent 786fd2f commit 6888cb8
Show file tree
Hide file tree
Showing 10 changed files with 616 additions and 22 deletions.
16 changes: 13 additions & 3 deletions scripts/src/github-api.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
import { describe, it, expect } from 'vitest';
import { isSuccessComment, isPRModificationFailureComment } from './github-api';
import type { GitHubComment } from './types';
import { describe, it, expect, vi } from 'vitest';
import { isSuccessComment, isPRModificationFailureComment, fetchBotActivities } from './github-api';

Check failure on line 2 in scripts/src/github-api.test.ts

View workflow job for this annotation

GitHub Actions / lint

'fetchBotActivities' is defined but never used
import type { GitHubComment, GitHubPR } from './types';

Check failure on line 3 in scripts/src/github-api.test.ts

View workflow job for this annotation

GitHub Actions / lint

'GitHubPR' is defined but never used

// Mock node-fetch
vi.mock('node-fetch', () => ({
default: vi.fn()
}));

// Mock fs
vi.mock('fs', () => ({
appendFileSync: vi.fn()
}));

describe('Comment detection', () => {
it('should detect issue success comment', () => {
Expand Down
27 changes: 25 additions & 2 deletions scripts/src/github-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,11 @@ async function processPRComments(pr: GitHubPR): Promise<Activity[]> {
const activities: Activity[] = [];
const comments = await fetchAllPages<GitHubComment>(pr.comments_url);

// Fetch PR details to get state and merged status
const baseUrl = `https://api.github.com/repos/${REPO_OWNER}/${REPO_NAME}`;
const prResponse = await fetchWithAuth(`${baseUrl}/pulls/${pr.number}`);

Check failure on line 153 in scripts/src/github-api.ts

View workflow job for this annotation

GitHub Actions / lint

Invalid type "number" of template literal expression
const prDetails = prResponse.data;

for (let i = 0; i < comments.length; i++) {
const comment = comments[i];
if (comment && isPRModificationComment(comment)) {
Expand All @@ -158,15 +163,33 @@ async function processPRComments(pr: GitHubPR): Promise<Activity[]> {
const resultComment = successComment ?? failureComment;

if (resultComment !== undefined) {
const status = successComment !== undefined ? 'success' : 'failure';
// Determine PR status based on whether PR was created and its state
let status: Activity['status'];
if (!successComment) {
status = 'no_pr';
} else if (prDetails.merged) {
status = 'pr_merged';
} else if (prDetails.state === 'closed') {
status = 'pr_closed';
} else {
status = 'pr_open';
}

const timestamp = new Date(resultComment.created_at).toLocaleString();
const statusText = {
'no_pr': 'No PR',
'pr_open': 'PR Open',
'pr_merged': 'PR Merged',
'pr_closed': 'PR Closed'
}[status];

activities.push({
id: `pr-${String(pr.number)}-${String(comment.id)}`,
type: 'pr',
status,
timestamp: resultComment.created_at,
url: resultComment.html_url,
title: `PR ${status} ${timestamp} -- ${pr.title}`,
title: `${statusText} ${timestamp} -- ${pr.title}`,
description: pr.body ? (pr.body.slice(0, 500) + (pr.body.length > 500 ? '...' : '')) : 'No description provided',
});
}
Expand Down
4 changes: 3 additions & 1 deletion scripts/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ export interface GitHubPR {
comments_url: string;
comments: number;
body?: string;
state?: string;
merged?: boolean;
}

export interface ApiResponse {
Expand All @@ -37,7 +39,7 @@ export interface ApiResponse {
export interface Activity {
id: string;
type: 'issue' | 'pr';
status: 'success' | 'failure';
status: 'success' | 'failure' | 'no_pr' | 'pr_open' | 'pr_merged' | 'pr_closed';
timestamp: string;
url: string;
title: string;
Expand Down
17 changes: 17 additions & 0 deletions src/App.css
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,23 @@ input[type="date"]::-webkit-calendar-picker {
word-break: break-word;
}

.activity-item.no_pr {
border-left: 4px solid #ffffff;
}

.activity-item.pr_open {
border-left: 4px solid #22c55e;
}

.activity-item.pr_merged {
border-left: 4px solid #a855f7;
}

.activity-item.pr_closed {
border-left: 4px solid #ef4444;
}

/* Legacy status styles */
.activity-item.success {
border-left: 4px solid #4caf50;
}
Expand Down
60 changes: 46 additions & 14 deletions src/components/ActivityList.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,38 +18,70 @@ describe('ActivityList', () => {
const mockActivities: BotActivity[] = [
{
id: '1',
type: 'issue',
status: 'success',
timestamp: '2023-11-28T12:00:00Z',
type: 'pr',
status: 'no_pr',
timestamp: '2024-01-01T12:00:00Z',
url: 'https://github.com/example/1',
title: 'ISSUE success 11/28/2023, 12:00:00 PM -- Test Issue 1',
description: 'Successfully resolved issue',
title: 'No PR opened',
description: 'No PR was created',
},
{
id: '2',
type: 'pr',
status: 'failure',
timestamp: '2023-11-28T13:00:00Z',
status: 'pr_open',
timestamp: '2024-01-02T12:00:00Z',
url: 'https://github.com/example/2',
title: 'PR failure 11/28/2023, 1:00:00 PM -- Test PR 1',
description: 'Failed to modify PR',
title: 'PR opened',
description: 'PR is open',
},
{
id: '3',
type: 'pr',
status: 'pr_merged',
timestamp: '2024-01-03T12:00:00Z',
url: 'https://github.com/example/3',
title: 'PR merged',
description: 'PR was merged',
},
{
id: '4',
type: 'pr',
status: 'pr_closed',
timestamp: '2024-01-04T12:00:00Z',
url: 'https://github.com/example/4',
title: 'PR closed',
description: 'PR was closed without merging',
},
];

it('renders activities correctly', () => {
render(<ActivityList activities={mockActivities} />);

// Check if activities are rendered
expect(screen.getByText('ISSUE success 11/28/2023, 12:00:00 PM -- Test Issue 1')).toBeInTheDocument();
expect(screen.getByText('PR failure 11/28/2023, 1:00:00 PM -- Test PR 1')).toBeInTheDocument();
expect(screen.getByText('Successfully resolved issue')).toBeInTheDocument();
expect(screen.getByText('Failed to modify PR')).toBeInTheDocument();
expect(screen.getByText('No PR opened')).toBeInTheDocument();
expect(screen.getByText('PR opened')).toBeInTheDocument();
expect(screen.getByText('PR merged')).toBeInTheDocument();
expect(screen.getByText('PR closed')).toBeInTheDocument();

// Check if links are rendered correctly
const links = screen.getAllByText('View on GitHub');
expect(links).toHaveLength(2);
expect(links).toHaveLength(4);
expect(links[0]).toHaveAttribute('href', 'https://github.com/example/1');
expect(links[1]).toHaveAttribute('href', 'https://github.com/example/2');
expect(links[2]).toHaveAttribute('href', 'https://github.com/example/3');
expect(links[3]).toHaveAttribute('href', 'https://github.com/example/4');
});

it('applies correct status classes to activities', () => {
render(<ActivityList activities={mockActivities} />);

const items = screen.getAllByTestId('activity-item');
expect(items).toHaveLength(4);

expect(items[0]).toHaveClass('activity-item', 'no_pr');
expect(items[1]).toHaveClass('activity-item', 'pr_open');
expect(items[2]).toHaveClass('activity-item', 'pr_merged');
expect(items[3]).toHaveClass('activity-item', 'pr_closed');
});

it('renders empty state correctly', () => {
Expand Down
2 changes: 1 addition & 1 deletion src/components/ActivityList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export function ActivityList({ activities }: ActivityListProps): React.JSX.Eleme
<div>
<div className="activity-list">
{currentActivities.map((activity) => (
<div key={activity.id} className={`activity-item ${activity.status}`}>
<div key={activity.id} data-testid="activity-item" className={`activity-item ${activity.status}`}>
<div className="activity-header">
<span className="activity-title">{activity.title}</span>
</div>
Expand Down
153 changes: 153 additions & 0 deletions src/services/github-api-pr.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { fetchBotActivities } from './github-api';
import type { GitHubPR } from './types';

// Mock node-fetch
vi.mock('node-fetch', () => ({
default: vi.fn()
}));

// Mock fs
vi.mock('fs', () => ({
default: {
appendFileSync: vi.fn()
},
appendFileSync: vi.fn()
}));

describe('PR status handling', () => {
let fetch: ReturnType<typeof vi.fn>;

beforeEach(async () => {
fetch = vi.mocked(await import('node-fetch')).default;

Check failure on line 22 in src/services/github-api-pr.test.ts

View workflow job for this annotation

GitHub Actions / build

Type 'typeof fetch' is missing the following properties from type 'Mock<Procedure>': getMockName, mockName, mock, mockClear, and 13 more.
fetch.mockReset();
process.env.GITHUB_TOKEN = 'test-token';
});

it('should handle PR states correctly', async () => {
// Mock PR with comments
const mockPR: GitHubPR = {
number: 123,
title: 'Test PR',
html_url: 'https://github.com/All-Hands-AI/OpenHands/pull/123',
comments_url: 'https://api.github.com/repos/All-Hands-AI/OpenHands/issues/123/comments',
comments: 2,
body: 'Test PR description'
};

// Mock comments indicating bot activity
const mockComments = [
{
id: 1,
body: 'OpenHands started fixing the issue',
html_url: 'https://github.com/All-Hands-AI/OpenHands/pull/123#comment-1',
created_at: '2024-01-01T00:00:00Z',
user: { login: 'openhands-agent' }
},
{
id: 2,
body: 'A potential fix has been generated',
html_url: 'https://github.com/All-Hands-AI/OpenHands/pull/123#comment-2',
created_at: '2024-01-01T00:01:00Z',
user: { login: 'openhands-agent' }
}
];

// Test different PR states
const prStates = [
{ state: 'open', merged: false, expectedStatus: 'pr_open' },
{ state: 'closed', merged: true, expectedStatus: 'pr_merged' },
{ state: 'closed', merged: false, expectedStatus: 'pr_closed' }
];

for (const { state, merged, expectedStatus } of prStates) {
// Mock API responses
fetch.mockImplementation((url: string) => {
if (url.includes('/comments')) {
return Promise.resolve({
ok: true,
json: () => Promise.resolve(mockComments),
headers: { get: () => null }
});
} else if (url.includes('/pulls/123')) {
return Promise.resolve({
ok: true,
json: () => Promise.resolve({ ...mockPR, state, merged }),
headers: { get: () => null }
});
} else if (url.includes('/issues')) {
return Promise.resolve({
ok: true,
json: () => Promise.resolve([{ ...mockPR, pull_request: {} }]),
headers: { get: () => null }
});
}
return Promise.reject(new Error(`Unexpected URL: ${url}`));
});

const activities = await fetchBotActivities();
expect(activities).toHaveLength(1);
expect(activities[0].status).toBe(expectedStatus);
expect(activities[0].type).toBe('pr');
}
});

it('should handle no PR case correctly', async () => {
// Mock PR with comments where bot failed to create PR
const mockPR: GitHubPR = {
number: 123,
title: 'Test PR',
html_url: 'https://github.com/All-Hands-AI/OpenHands/pull/123',
comments_url: 'https://api.github.com/repos/All-Hands-AI/OpenHands/issues/123/comments',
comments: 2,
body: 'Test PR description'
};

// Mock comments indicating bot failure
const mockComments = [
{
id: 1,
body: 'OpenHands started fixing the issue',
html_url: 'https://github.com/All-Hands-AI/OpenHands/pull/123#comment-1',
created_at: '2024-01-01T00:00:00Z',
user: { login: 'openhands-agent' }
},
{
id: 2,
body: 'The workflow to fix this issue encountered an error',
html_url: 'https://github.com/All-Hands-AI/OpenHands/pull/123#comment-2',
created_at: '2024-01-01T00:01:00Z',
user: { login: 'openhands-agent' }
}
];

// Mock API responses
fetch.mockImplementation((url: string) => {
if (url.includes('/comments')) {
return Promise.resolve({
ok: true,
json: () => Promise.resolve(mockComments),
headers: { get: () => null }
});
} else if (url.includes('/pulls/123')) {
return Promise.resolve({
ok: true,
json: () => Promise.resolve({ ...mockPR, state: 'open', merged: false }),
headers: { get: () => null }
});
} else if (url.includes('/issues')) {
return Promise.resolve({
ok: true,
json: () => Promise.resolve([{ ...mockPR, pull_request: {} }]),
headers: { get: () => null }
});
}
return Promise.reject(new Error(`Unexpected URL: ${url}`));
});

const activities = await fetchBotActivities();
expect(activities).toHaveLength(1);
expect(activities[0].status).toBe('no_pr');
expect(activities[0].type).toBe('pr');
});
});
Loading

0 comments on commit 6888cb8

Please sign in to comment.