Skip to content

Commit f67caa3

Browse files
authored
fix(pr-status): address claude-review feedback post-merge (#571 follow-up)
## Summary - Fix misleading "All clear" during PROOF9 loading (show skeleton) - Surface proof API errors in PROOF9 section - Handle externally merged PRs (show success banner) - Add ciPending test + 3 edge case tests (760 total) Relates to #571
1 parent ba8f708 commit f67caa3

2 files changed

Lines changed: 54 additions & 6 deletions

File tree

web-ui/__tests__/components/review/PRStatusPanel.test.tsx

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@ const failingCIChecks = [
3434
{ name: 'tests', status: 'completed', conclusion: 'failure' },
3535
];
3636

37+
const pendingCIChecks = [
38+
{ name: 'tests', status: 'in_progress', conclusion: null },
39+
];
40+
3741
const basePRStatus = {
3842
ci_checks: successfulCIChecks,
3943
review_status: 'approved',
@@ -79,11 +83,20 @@ const proofStatusWithOpenReqs = {
7983

8084
// ── Helpers ───────────────────────────────────────────────────────────────────
8185

82-
const setupSWRMock = (prStatus: object, proofStatus: object) => {
86+
const setupSWRMock = (
87+
prStatus: object | undefined,
88+
proofStatus: object | undefined,
89+
options?: { proofLoading?: boolean; proofError?: unknown }
90+
) => {
8391
mockUseSWR.mockImplementation((key: unknown) => {
8492
const keyStr = typeof key === 'string' ? key : '';
8593
if (keyStr.includes('/api/v2/proof/status')) {
86-
return { data: proofStatus, error: undefined, isLoading: false, mutate: jest.fn() } as any;
94+
return {
95+
data: proofStatus,
96+
error: options?.proofError,
97+
isLoading: options?.proofLoading ?? false,
98+
mutate: jest.fn(),
99+
} as any;
87100
}
88101
return { data: prStatus, error: undefined, isLoading: false, mutate: jest.fn() } as any;
89102
});
@@ -185,4 +198,32 @@ describe('PRStatusPanel — PROOF9-gated merge button', () => {
185198
expect(screen.getByText(/ci checks failing/i)).toBeInTheDocument();
186199
expect(screen.getByText('Fix critical bug')).toBeInTheDocument();
187200
});
201+
202+
it('shows "Waiting for CI checks" when CI checks are pending', () => {
203+
setupSWRMock({ ...basePRStatus, ci_checks: pendingCIChecks }, cleanProofStatus);
204+
render(<PRStatusPanel {...defaultProps} />);
205+
expect(screen.getByText(/waiting for ci checks/i)).toBeInTheDocument();
206+
expect(screen.getByRole('button', { name: /^merge$/i })).toBeDisabled();
207+
});
208+
209+
it('shows PROOF9 loading skeleton instead of "All clear" while proof data loads', () => {
210+
setupSWRMock(basePRStatus, undefined, { proofLoading: true });
211+
render(<PRStatusPanel {...defaultProps} />);
212+
expect(screen.queryByText(/all clear/i)).not.toBeInTheDocument();
213+
expect(screen.getByRole('button', { name: /^merge$/i })).toBeDisabled();
214+
});
215+
216+
it('shows PROOF9 error message when proof API fails', () => {
217+
setupSWRMock(basePRStatus, undefined, { proofError: new Error('Network error') });
218+
render(<PRStatusPanel {...defaultProps} />);
219+
expect(screen.getByText(/unable to load proof9 status/i)).toBeInTheDocument();
220+
expect(screen.getByRole('button', { name: /^merge$/i })).toBeDisabled();
221+
});
222+
223+
it('shows success banner when PR was merged externally', () => {
224+
setupSWRMock({ ...basePRStatus, merge_state: 'merged' }, cleanProofStatus);
225+
render(<PRStatusPanel {...defaultProps} />);
226+
expect(screen.getByText(/merged successfully/i)).toBeInTheDocument();
227+
expect(screen.queryByRole('button', { name: /merge/i })).not.toBeInTheDocument();
228+
});
188229
});

web-ui/src/components/review/PRStatusPanel.tsx

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ export function PRStatusPanel({ prNumber, workspacePath }: PRStatusPanelProps) {
9797
}
9898
);
9999

100-
const { data: proofData } = useSWR<ProofStatusResponse>(
100+
const { data: proofData, error: proofError, isLoading: proofLoading } = useSWR<ProofStatusResponse>(
101101
proofKey,
102102
() => proofApi.getStatus(workspacePath),
103103
{ refreshInterval: merged ? 0 : 15_000 }
@@ -121,6 +121,7 @@ export function PRStatusPanel({ prNumber, workspacePath }: PRStatusPanelProps) {
121121
);
122122

123123
const ciPassing = !ciFailing && !ciPending;
124+
const alreadyMerged = merged || data?.merge_state === 'merged';
124125
const canMerge = !!data && !!proofData && openRequirements.length === 0 && ciPassing;
125126

126127
// ── Merge handler ─────────────────────────────────────────────────────────
@@ -204,7 +205,13 @@ export function PRStatusPanel({ prNumber, workspacePath }: PRStatusPanelProps) {
204205
{/* PROOF9 gate section */}
205206
<div className="flex flex-col gap-1.5">
206207
<span className="text-sm font-medium">PROOF9</span>
207-
{openRequirements.length === 0 ? (
208+
{proofLoading ? (
209+
<div className="h-4 animate-pulse rounded bg-muted" />
210+
) : proofError && !proofData ? (
211+
<p className="text-xs text-muted-foreground">
212+
Unable to load PROOF9 status — merge blocked until resolved.
213+
</p>
214+
) : openRequirements.length === 0 ? (
208215
<p className="flex items-center gap-1 text-xs text-muted-foreground">
209216
<CheckmarkCircle01Icon className="h-3 w-3 text-green-600" />
210217
All clear
@@ -233,7 +240,7 @@ export function PRStatusPanel({ prNumber, workspacePath }: PRStatusPanelProps) {
233240
)}
234241

235242
{/* Blocking messages */}
236-
{data && (ciFailing || ciPending) && !merged && (
243+
{data && (ciFailing || ciPending) && !alreadyMerged && (
237244
<p className="text-xs text-amber-600">
238245
{ciFailing ? 'CI checks failing' : 'Waiting for CI checks'}
239246
</p>
@@ -247,7 +254,7 @@ export function PRStatusPanel({ prNumber, workspacePath }: PRStatusPanelProps) {
247254
)}
248255

249256
{/* Success banner or Merge button */}
250-
{merged ? (
257+
{alreadyMerged ? (
251258
<div className="flex items-center gap-1 rounded bg-green-50 px-3 py-2 text-xs text-green-700">
252259
<CheckmarkCircle01Icon className="h-3 w-3" />
253260
PR #{prNumber} merged successfully

0 commit comments

Comments
 (0)